Problem/Motivation

EntityResolverManager instantiates objects unnecessarily which is super dangerous since router rebuild occur during kernel destruct event when many services become unavailable - eg. current_user which would currently cause a route rebuild to fail if the simpletest module is enabled since SimpletestResultsForm uses the cache_context service during its construction.

This is critical because router rebuild errors like this are hard to detect and test for.

Proposed resolution

Creating the objects is actually unnecessary since reflection will work just fine with class names.

Remaining tasks

Write patch

User interface changes

None

API changes

None

CommentFileSizeAuthor
#26 2300131.26-interdiff.txt1.75 KBBerdir
#26 2300131.26.patch14.11 KBBerdir
#19 2300131.19-interdiff.txt642 bytesBerdir
#19 2300131.19.patch14.08 KBBerdir
#17 2300131.16.patch14.08 KBBerdir
#3 2300131.3.patch13.99 KBalexpott
#2 2234159.1.patch22.13 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Yeah using the controller resolver probably won't work here then, we need to manually figure out which class we want to reflect against.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Active » Needs review
FileSize
2234159.1.patch22.13 KB

First attempt at a patch - we have a problem with the service:method notation...

    $count = substr_count($controller, ':');
    if ($count == 1) {
      // Controller in the service:method notation. Should we be getting the
      // service from the container? Or can we get the definition somehow?
      // Considering that this is called during the kernel destruct event this
      // is very dangerous as the controller could depend on services that can
      // not exist at this point.
      list($class_or_service, $method) = explode(':', $controller, 2);
      return array($this->classResolver->getInstanceFromDefinition($class_or_service), $method);
    }

From the patch. Otherwise it is totally possible to not actually construct the objects and use reflection to discover the entities to upcast.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
2300131.3.patch13.99 KB

Patch in #2 includes another patch :(

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Maybe the class resolver could get a util to return a reflection object?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

Yeah I though about that... but that we would need to do that for the controller resolver too

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Could this even speed up the rebuilding process a bit?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Though I think these are tasks for follow ups.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs work

Contrib and custom code is going to be interacting with route rebuilding as well, and the requirement not to instantiate objects isn't documented anywhere afaik.

Is there another place we could do the route rebuild (right before kernel destruct for example?) so it doesn't run into this problem? If there's a performance improvement we could also make this change, but doesn't look like the right fix to the actual problem.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Needs review

So the reason we have the problem is that we currently have a synthetic request service and the request scope - but this is removed in #2284103: Remove the request from the container. Once that is in I don't think we need to add a comment about anything.

In fact this patch makes the EntityResolver better by allowing the use of services in more places for entity resolving.

Berdir queued 3: 2300131.3.patch for re-testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Instanciating all the services was quite good to find all kind of triggered code during construction time but I don't think
we should actually use that problem as a feature.

dawehner queued 3: 2300131.3.patch for re-testing.

Berdir queued 3: 2300131.3.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 3: 2300131.3.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review

Re-roll, let's see if that worked. Not sure if there anything else to do here?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
2300131.16.patch14.08 KB

A patch would help.

Status: Needs review » Needs work

The last submitted patch, 17: 2300131.16.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
2300131.19.patch14.08 KB
2300131.19-interdiff.txt642 bytes

So the unit test was actually failing on something valid, crazy stuff ;)

Status: Needs review » Needs work

The last submitted patch, 19: 2300131.19.patch, failed testing.

Berdir queued 19: 2300131.19.patch for re-testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

Contrib and custom code is going to be interacting with route rebuilding as well, and the requirement not to instantiate objects isn't documented anywhere afaik.

Flipping the question around... @catch why should they expect their controllers to be instantiated during a route rebuild?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Yes, I don't see how that would make sense either. We would have to document it if we wanted them to be called, but not the other way round...

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

This certainly was not the case in the earlier days of the routing system, this instantiation was done to be able to figure out
whether you wanted to upcast or not. The init. though was always just a pure implementation detail, if you ask me. The upcasting process certainly
worked without it. 

  1. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
@@ -46,49 +37,78 @@ class EntityResolverManager {
+   * Rather than creating an instance of every controller determine the class
+   * and method that would be used. This is not possible for the service:method
+   * notation.

    We should explain that the container on runtime doesn't have that information. 

  2. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
@@ -46,49 +37,78 @@ class EntityResolverManager {
+      // Controller in the service:method notation. Should we be getting the
+      // service from the container? Or can we get the definition somehow?
+      // Considering that this is called during the kernel destruct event this
+      // is very dangerous as the controller could depend on services that can
+      // not exist at this point.

    Not for this issue but I wonder whether we could allow for special cases (route rebuilding) to able to fetch a containerBuilder instance which has that information. Maybe we could even dump a Builder container, in case if needed.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
2300131.26.patch14.11 KB
2300131.26-interdiff.txt1.75 KB

Improved those comments a bit.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

This is good to go now.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 0ce0a35 on 8.0.x 
    Issue #2300131 by Berdir, alexpott: Fixed EntityResolverManager...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.