Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
$this->moduleHandler->invokeAll(...)
Comment | File | Size | Author |
---|---|---|---|
#29 | entity-access-2041333-29.patch | 6.11 KB | tim.plunkett |
#29 | interdiff.txt | 1.67 KB | tim.plunkett |
#27 | entity-2041333-27.patch | 5.75 KB | tim.plunkett |
#27 | interdiff.txt | 945 bytes | tim.plunkett |
#25 | interdiff.txt | 1.03 KB | tim.plunkett |
Comments
Comment #1
tstoecklerHere we go.
In theory the rename of $this->entity_type to $this->entityType is unrelated, but I couldn't resist...
Comment #3
dawehnerIt fails because the BLockAccessController already has a constructor.
Comment #4
tstoecklerThanks for that hint! NodeAccessController has one as well. Also, apparently it's 'module_handler' and not 'module_handler.cached'.
Comment #6
tstoecklerOh 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.
Comment #7
dawehner****
Missing docs for the module handler (wrong order in the last case).
Comment #8
tstoecklerGood catch.
Comment #9
tstoecklerOh, and because this changes constructors, this is officially an API change...
Comment #10
dawehnerThank you!
Comment #11
alexpottPatch no longer applies
Comment #12
tstoecklerYay, no more BCDecorator! This was the conflict:
Comment #13
tstoecklerBack to RTBC. To prove I didn't mess anything up, I diffed the two patches.
Comment #14
webchickCommitted and pushed to 8.x. Thanks!
Comment #15
tim.plunkettWow, 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.
Also, is this just completely out of scope change done just for fun? Nothing changed here.
Comment #16
webchickOops, 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.
Comment #17
tim.plunkettI'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.
Comment #18
BerdirYeah, 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.
Comment #19
tstoecklerRe #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
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! :-)
Comment #20
tim.plunkettHere we are. The interdiff is much messier than the patch, I'd say.
Comment #22
twistor CreditAttribution: twistor commentedWhy can't we just call getController() and then set the module handler before returning?
Comment #23
Berdir"self", is that a thing? never seen before.
Comment #24
twistor CreditAttribution: twistor commentedIt'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.
Comment #25
tim.plunkettI borrowed @return self from Guzzle initially, but now its all over.
Good call on using getController.
Also fixed a typo (fixing the failing test).
Comment #27
tim.plunkettUgh I fixed this locally
Comment #28
dawehnerI 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 :)
Comment #29
tim.plunkettAhem. :)
Comment #30
dawehnerHa, this works
Comment #31
tim.plunkett#29: entity-access-2041333-29.patch queued for re-testing.
Comment #32
XanoUpping to major, since this blocks #2057377: Implement hook_entity_access() and hook_entity_create_access() and #1839516: Introduce entity operation providers.
Comment #33
alexpottCommitted 9be3ce8 and pushed to 8.x. Thanks!