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

tstoeckler’s picture

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.

dawehner’s picture

It fails because the BLockAccessController already has a constructor.

tstoeckler’s picture

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.

tstoeckler’s picture

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.

dawehner’s picture

+++ 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).

tstoeckler’s picture

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

Good catch.

tstoeckler’s picture

Issue tags:+API change

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

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Thank you!

alexpott’s picture

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

Patch no longer applies

tstoeckler’s picture

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
tstoeckler’s picture

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.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

tim.plunkett’s picture

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.

webchick’s picture

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.

tim.plunkett’s picture

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.

Berdir’s picture

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.

tstoeckler’s picture

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! :-)

tim.plunkett’s picture

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.

twistor’s picture

+++ 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?

Berdir’s picture

+++ 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.

twistor’s picture

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.

tim.plunkett’s picture

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.

tim.plunkett’s picture

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

Ugh I fixed this locally

dawehner’s picture

+++ 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 :)

tim.plunkett’s picture

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

Ahem. :)

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Ha, this works

tim.plunkett’s picture

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

Xano’s picture

alexpott’s picture

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.