#Needs issue summary update

Just figured out that the failing of:

• Drupal\Tests\package_manager\Build\PackageInstallTest
• Drupal\Tests\package_manager\Build\PackageInstallSubmoduleTest
• Drupal\Tests\package_manager\Build\PackageUpdateTest

with the error message
Some modules have database schema updates to install. You should run the database update script immediately
was caused by the incorrect placement of the post_update hook function in the link.module file instead of the required link.post_update.php file.

Attached a static inline_entity_form--2025-01-04--mr-126.patch file, from the MR126 up to this point.
to be used with Composer Patches

Rebased.

Left comments and questions on the MR.

• Updated Issue Summary.
• Rebased the MR over the most recent 11.x branch.
• Resolved all GitLab threads.
• Added a post_update hook and a test for it.

The MR Gitlab pipeline is failing and I believe it has to do with some unrelated problem in the 11.x branch. Not setting this issue to "Needs Review" until this is solved.

vidorado → changed the visibility of the branch 11.x to hidden.

watergate → changed the visibility of the branch 11.x to hidden.

Believe feedback has been addressed

Hello Drupal Community
This merge request addresses an existing issue with tokens in attributes. Here’s a brief overview of the changes made:

• Allows Tokens for src and html Attributes: This will enhance flexibility in how tokens are utilized within these attributes.
• Added Support for href Attribute: In addition to the above, I've added support for tokens within the href attribute..

This implementation is built upon two patches provided by @danflanagan8.

I welcome any feedback or suggestions from the community, as I strive to ensure that this merge request meets our shared standards and needs.

Thank you for your consideration!

Best regards,
Richard Hoogstad

larowlan → closed merge request !3250

Committed to 11.x and backported to 11.1.x, 10.4.x and 10.5.x

Great to see this one finalized, mammoth effort over many years.

Couple of things:

1. +++ b/core/lib/Drupal/Core/Access/RouteProcessorCsrfAjax.php
   @@ -0,0 +1,73 @@
   + * Processes outbound ajax/nojs route to handle the CSRF token.
   

   This needs documentation to explain why this class is different from the parent.

2. +++ b/core/lib/Drupal/Core/Access/RouteProcessorCsrfAjax.php
   @@ -0,0 +1,73 @@
   +  public function processOutbound($route_name, Route $route, array &$parameters, BubbleableMetadata $bubbleable_metadata = NULL) {
   

   This method looks the same as in the parent?

I've added tests, but I can't get them to fail even after undoing the fix... 🤔

It seems the original entity is already fine in D11.

Could you take a look, @dieterholvoet?

vidorado → changed the visibility of the branch 2140179-entity-original-gets-stale to hidden.

vidorado → changed the visibility of the branch 11.x to hidden.

@igorgoncalves yes, indeed the two strings of urls are different, that is what we need here. Do I need to make changes to test files, to remove the warnings. I don't have much knowledge of phpunit tests, but can look into it.

Hi Sayan, thanks for the effort.
Despite the fact that tests have "Pass" status, there's one warning left, and checking the detail says:

Drupal\Tests\Core\Utility\UnroutedUrlAssemblerTest::testAssembleWithExternalUrl with data set #7
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'https://example.com/test?foo'
+'https://example.com/test?foo='

which sounds like pretty much what we are testing here, isnt?

📌 Create placeholders for more things Active isn't enough for manual testing at least with stock Umami. I think for it to work, we'd need to be rendering entities in placeholders, then it would help with entity references - that currently doesn't happen. Will open another issue. Can still add test coverage here to demonstrate the concept.

@bhanu951:

Thanks for those final changes. I have no more suggestions.

I updated the change record. It still had the message from before the usability review in Comment #187.

I also tested manually and got something that looks a lot like the screenshot in the issue summary. (That is already up to date.)

I crossed off the last of the "Remaining tasks" in the issue summary.

@sagarmohite0031:

Thanks for the additional testing. But this issue already has up-to-date screenshots in the issue summary.

If you search for issues with the Needs screenshots → tag, you will find plenty of issues that need new screenshots. (You will probably find some that already have good screenshots, but someone forgot to remove the tag.)

Instead of uploading screenshots, it helps more to embed them. This issue is a good example: see the "before" and "after" screenshots under "User interface changes" in the issue summary. See Add screenshots to an issue → for more detailed instructions.

This is still relevant. Of course the code need rebase but it is a treasure and should not be burried in a closed issue.

Re-rolled for 11.x

Hello,
Tested and verified on Drupal 11,
MR applied successfully
Working as expected
Attaching before and after screenshots-

@benjifisher : I made those changes, please re-review it. Thanks.

@bhanu951:

Thanks for the updates. Now the test passes (locally and in GitLab CI). Yay!

I just have some suggestions for code cleanup on the MR. Back to NW for that.

Pushed a branch. It's going to need 📌 Try to replace path alias preloading with lazy generation Active to be able to show any profiling numbers and for manual testing.

I think kernel test coverage of this should be pretty straightforward. We need a hook_entity_load() test implementation that logs how many times it's called (might already exist), create three entities, then load the three entities inside a fiber each, and see how many times the hook implementation is called.

Added a small nit, but will leave it at NR

Thanks @benjifisher for the review, addressed your feedback.

I think it is ready for re-review.

Ten years later...

The main barrier to pursuing this, apart from ::__get() or anything else is that when you call Entity::load()/EntityStorage::load() you either get an entity or FALSE back, you never get an entity object that might or might not exist in the database. That makes the original approach here very hard to reconcile with entity caching - we never want to run a query just to check if we have an entity or not, when we could just get it from cache anyway.

📌 Try to replace path alias preloading with lazy generation Active has a proof of concept for changing how path alias preloading works, it relies on Fibers which didn't exist until PHP 8.1. By doing so it doesn't rely on any changes to entity classes, magic methods etc. and I think the same thing can work here.

Let's say we have three independent entity loading calls in different placeholders (or any code that can run inside a Fiber, which will be most of it after 🌱 Adopt the Revolt event loop for async task orchestration Active .

// Placeholder 1.
Node::load(4);
// Placeholder 2.
Node::load(3);
// Placeholder 3.
Node::loadMultiple(1, 2, 3);

With the Fibers approach, when each of these calls happen, we can check the static cache, and return as normal in those cases.

When we get a cache miss, we can add the entity ID to a class property that holds 'a list of entities that need to be loaded' (i.e. haven't been returned from static cache), and suspend the Fiber.

So during the page request, node 4 gets added to the class property first, the fiber is suspended, we go to the next placeholder, same thing happens for node 3, go to the third placeholder, 1 and 2 are added (3 is already in there).

Then we run out of placeholders to loop through and end up back at the first one. We now have nodes 1, 2, 3, and 4 all in the 'to be loaded' property, then we can run the rest of multiple loading - first check the persistent cache, then the database. When we reach the later placeholders, they check the static cache again, find that the entity was already loaded, and go ahead.

What would have been three separate database loading operations now gets collapsed into one.

What this won't affect is the following code in a single placeholder:

Node::load(1);
Node::load(2);

e.g. when we Fiber::suspend(), we can only get back to Node::load(1) and complete that before going to Node::load(2).

But if this happens in two placeholders:

// Placeholder 1.
Node::load(4);
Node::load(3);
// Placeholder 2.
Node::load(2);
Node::load(1);

In this case, the approach would mean that node 4 and 2 are multiple loaded together, and then node 3 and 1 are multiple loaded together, so two multiple loads instead of four single loads.

And the worst case is that we Fiber::suspend(), there are no other entities of the same type to load, and we end up back where we were - then it's effectively a no-op and will have the overhead of only a couple of cheap function calls.

I have applied the MR!7 and there are no errors

NW for the failing test. See my comment on the MR for how to fix it (I think).

Also (as I said on the MR) move the test to the system module.

@bhanu951: It looks as though you updated the target of the MR to 11.x, but you never updated the feature branch.

I just rebased the feature branch on 11.x and force-pushed. That should trigger the automated tests.

I think the test coverage is already there. Back to NR.