Once #1979094: Separate create access operation entity access controllers to avoid costly EntityNG instantiation the module_invoke_all() calls should be replaced by an injected module handler and a call to

<?php
$this
->moduleHandler->invokeAll(...)
?>
Files: 
CommentFileSizeAuthor
#29 entity-access-2041333-29.patch6.11 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 59,268 pass(es).
[ View ]
#29 interdiff.txt1.67 KBtim.plunkett
#27 entity-2041333-27.patch5.75 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,030 pass(es).
[ View ]
#27 interdiff.txt945 bytestim.plunkett
#25 interdiff.txt1.03 KBtim.plunkett
#25 entity-2041333-25.patch5.75 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,026 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#20 access-controller-2041333-20.patch6.1 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,306 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#20 interdiff.txt8.29 KBtim.plunkett
#13 diffdiff.txt1.77 KBtstoeckler
#12 2041333-12-entity-access-injection.patch8.13 KBtstoeckler
PASSED: [[SimpleTest]]: [MySQL] 58,486 pass(es).
[ View ]
#8 2041333-8-entity-access-injection.patch8.16 KBtstoeckler
PASSED: [[SimpleTest]]: [MySQL] 58,314 pass(es).
[ View ]
#8 interdiff-6-8.txt2.48 KBtstoeckler
#6 2041333-6-entity-access-injection.patch7.58 KBtstoeckler
PASSED: [[SimpleTest]]: [MySQL] 58,189 pass(es).
[ View ]
#6 interdiff-4-6.txt1.58 KBtstoeckler
#4 2041333-4-entity-access-injection.patch6.28 KBtstoeckler
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#4 interdiff-1-4.txt997 byteststoeckler
#1 2041333-1-entity-access-injection.patch3.03 KBtstoeckler
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Comments

Status:Postponed» Needs review
StatusFileSize
new3.03 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Here we go.

In theory the rename of $this->entity_type to $this->entityType is unrelated, but I couldn't resist...

Status:Needs review» Needs work

The last submitted patch, 2041333-1-entity-access-injection.patch, failed testing.

It fails because the BLockAccessController already has a constructor.

Status:Needs work» Needs review
StatusFileSize
new997 bytes
new6.28 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Thanks for that hint! NodeAccessController has one as well. Also, apparently it's 'module_handler' and not 'module_handler.cached'.

Status:Needs review» Needs work

The last submitted patch, 2041333-4-entity-access-injection.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.58 KB
new7.58 KB
PASSED: [[SimpleTest]]: [MySQL] 58,189 pass(es).
[ View ]

Oh god, this is embarassing...

The EntityNG hunk is unrelated strictly speaking but I found it when debugging and I think it's clearer this way.

+++ a/core/lib/Drupal/Core/Entity/EntityAccessController.php
@@ -53,7 +53,7 @@ public function __construct($entity_type, ModuleHandlerInterface $module_handler
-    new static(
+    return new static(

****

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.php
@@ -30,13 +32,31 @@ class EntityAccessController implements EntityAccessControllerInterface {
    * Constructs an access controller instance.
    *
    * @param string $entity_type
    *   The entity type of the access controller instance.
    */
...
+  public function __construct($entity_type, ModuleHandlerInterface $module_handler) {
+++ b/core/modules/block/lib/Drupal/block/BlockAccessController.php
@@ -35,8 +36,9 @@ class BlockAccessController extends EntityAccessController implements EntityCont
    * @param \Drupal\Core\Path\AliasManagerInterface $alias_manager
    *   The alias manager.
    */
...
+  public function __construct($entity_type, ModuleHandlerInterface $module_handler, AliasManagerInterface $alias_manager) {
+++ b/core/modules/node/lib/Drupal/node/NodeAccessController.php
@@ -48,10 +41,10 @@ class NodeAccessController extends EntityAccessController implements NodeAccessC
    * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
    *   The module handler to invoke the alter hook with.
    */

Missing docs for the module handler (wrong order in the last case).

StatusFileSize
new2.48 KB
new8.16 KB
PASSED: [[SimpleTest]]: [MySQL] 58,314 pass(es).
[ View ]

Good catch.

Issue tags:+API change

Oh, and because this changes constructors, this is officially an API change...

Status:Needs review» Reviewed & tested by the community

Thank you!

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new8.13 KB
PASSED: [[SimpleTest]]: [MySQL] 58,486 pass(es).
[ View ]

Yay, no more BCDecorator! This was the conflict:

      // We grant access to the entity if both of these conditions are met:
      // - No modules say to deny access.
      // - At least one module says to grant access.
++<<<<<<< HEAD
+    $access = module_invoke_all($entity->entityType() . '_access', $entity, $operation, $account, $langcode);
++=======
+     $access = $this->moduleHandler->invokeAll($entity->entityType() . '_access', array($entity->getBCEntity(), $operation, $account, $langcode));
++>>>>>>> PATCH
      if (($return = $this->processAccessHookResults($access)) === NULL) {
        // No module had an opinion about the access, so let's the access

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new1.77 KB

Back to RTBC. To prove I didn't mess anything up, I diffed the two patches.

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

Wow, really? We *just* finished removing the module handler from the constructor of the entity form controller because it was so cumbersome for implementations.

This was a needless API change, it could have used setter injection exactly like EntityFormController and been done with it.

+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
@@ -413,14 +413,12 @@ public function isEmpty() {
+    $access_controller = \Drupal::entityManager()->getAccessController($this->entityType);
+
     if ($operation == 'create') {
-      return \Drupal::entityManager()
-        ->getAccessController($this->entityType)
-        ->createAccess($this->bundle(), $account);
+      return $access_controller->createAccess($this->bundle(), $account);
     }
-    return \Drupal::entityManager()
-      ->getAccessController($this->entityType)
-      ->access($this, $operation, $this->activeLangcode, $account);
+    return $access_controller->access($this, $operation, $this->activeLangcode, $account);

Also, is this just completely out of scope change done just for fun? Nothing changed here.

Status:Fixed» Needs review

Oops, sorry! I was looking for quick and easy things in the RTBC queue and this looked like one on the surface.

Reverted for now. Back to needs review.

I'm sorry. I just reread my comment and it sounds pretty entitled and angry. I didn't mean to be, it's the end of a long week.
I'll have a new patch tomorrow.

Status:Needs review» Needs work

Yeah, the problem with base classes and changing create()/createInstance() is that it completely kills existing child classes that are trying to play nice and implement createInstance()/create() too.

Setters are better, as long as we define that it's strongly recommended to extend from the base or another implementation and define adding new methods that have a default implementation to an interface as API addition.

Re #15, yeah that is unrelated strictly speaking, see #6. I had to put a breakpoint on the $access_controller anyway, and I didn't find any reason to revert that then afterwards.

I'm fine with setter injection, although I don't see this as strongly as @tim.plunkett or @Berdir. As you can see NodeAccessController was already using the same pattern so I don't think

it completely kills existing child classes that are trying to play nice and implement createInstance()/create() too

is actually a valid statement. But OK...

Not a big deal either way as this is a pretty trivial patch (once I stopped being stupid...), but it would have been easier to change this instead of reverting the entire patch.

Just to note, I'm AFK for 2 weeks basically as of now, so I won't re-roll this. That's certainly not because I'm put off or anything, though, to make that clear! :-)

Status:Needs work» Needs review
StatusFileSize
new8.29 KB
new6.1 KB
FAILED: [[SimpleTest]]: [MySQL] 58,306 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Here we are. The interdiff is much messier than the patch, I'd say.

Status:Needs review» Needs work

The last submitted patch, access-controller-2041333-20.patch, failed testing.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -315,7 +315,18 @@ public function getRenderController($entity_type) {
    */
   public function getAccessController($entity_type) {
-    return $this->getController($entity_type, 'access');
+    if (!isset($this->controllers['access'][$entity_type])) {
+      $class = $this->getControllerClass($entity_type, 'access');
+      if (in_array('Drupal\Core\Entity\EntityControllerInterface', class_implements($class))) {
+        $controller = $class::createInstance($this->container, $entity_type, $this->getDefinition($entity_type));
+      }
+      else {
+        $controller = new $class($entity_type);
+      }
+      $controller->setModuleHandler($this->moduleHandler);
+      $this->controllers['access'][$entity_type] = $controller;
+    }
+    return $this->controllers['access'][$entity_type];

Why can't we just call getController() and then set the module handler before returning?

+++ b/core/lib/Drupal/Core/Entity/EntityAccessControllerInterface.php
@@ -58,4 +59,15 @@ public function createAccess($entity_bundle = NULL, AccountInterface $account =
+   * @return self

"self", is that a thing? never seen before.

It's definitely a thing. Not sure if it's the correct thing. But, browsing through core, you can find it in PluginBags and Forms, so I guess it's Tim's thing. It's also in Guzzle and Doctrine. Alternatively, the database api uses the class name in long form.

Personally, I think 'self' makes more sense. It implies that the object is returning itself, with the class name it's not immediately obvious if the same instance is being returned.

Status:Needs work» Needs review
StatusFileSize
new5.75 KB
FAILED: [[SimpleTest]]: [MySQL] 58,026 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1.03 KB

I borrowed @return self from Guzzle initially, but now its all over.

Good call on using getController.

Also fixed a typo (fixing the failing test).

Status:Needs review» Needs work

The last submitted patch, entity-2041333-25.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new945 bytes
new5.75 KB
PASSED: [[SimpleTest]]: [MySQL] 58,030 pass(es).
[ View ]

Ugh I fixed this locally

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.php
@@ -244,4 +252,11 @@ protected function prepareUser(AccountInterface $account = NULL) {
+    $this->moduleHandler = $module_handler;
+  }
+++ b/core/lib/Drupal/Core/Entity/EntityAccessControllerInterface.php
@@ -58,4 +59,15 @@ public function createAccess($entity_bundle = NULL, AccountInterface $account =
+   * @return self
+   *   The entity form.
+   */
+  public function setModuleHandler(ModuleHandlerInterface $module_handler);

I do like using @return self, as it gives you some advantages over returning just the class itself, for example it works in a protected context, but why are we not returning it at the moment :)

StatusFileSize
new1.67 KB
new6.11 KB
PASSED: [[SimpleTest]]: [MySQL] 59,268 pass(es).
[ View ]

Ahem. :)

Status:Needs review» Reviewed & tested by the community

Ha, this works

#29: entity-access-2041333-29.patch queued for re-testing.

Status:Reviewed & tested by the community» Fixed

Committed 9be3ce8 and pushed to 8.x. Thanks!

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