Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

Status: Postponed » Needs review
FileSize
3.03 KB

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
FileSize
997 bytes
6.28 KB

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
FileSize
1.58 KB
7.58 KB

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

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
FileSize
8.13 KB

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
FileSize
1.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
FileSize
8.29 KB
6.1 KB

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
FileSize
5.75 KB
1.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
FileSize
945 bytes
5.75 KB

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

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.