Group content entity operations ~~hook~~ event #684
Conversation
pfrenssen
force-pushed
the
group-content-entity-operations-hook
branch
from
1c04e65 to
ccf35d4
Compare
|
This is the final PR in the |
|
In fact the argument is passed by value as a variable that contains the reference to an object (much like pointers). But when passing objects it shouldn't really matter if you have one or two variables as long as they point to the same object. edit: unless if you want to change the original object reference (e.g. have it reference another object). |
Co-authored-by: Maarten Segers <[email protected]>
…tions to an event.
pfrenssen
changed the title
|
@MPParsley I think this might be good to go but before merging I want to see a green build on my big project to see if there are possible regressions. I need to integrate the latest changes into my project (mainly #402) but I plan to tackle this right now so I will probably have news in ~2 hours. |
|
Got a green test: ec-europa/joinup-dev#2184, no regressions, let's get this in. I have one final small improvement waiting in #692. |
| public function getAccessResult(): AccessResultInterface { | ||
| $access = $this->access; | ||
|
|
||
| if ($access instanceof RefinableCacheableDependencyInterface) { |
There was a problem hiding this comment.
Maybe worth adding a comment - I'm not clear on this part?
There was a problem hiding this comment.
This ensures that our code adheres correctly to our interface and avoids a potential fatal error.
We are returning an AccessResultInterface object, and this doesn't offer ::addCacheableDependency() so we are not supposed to call this method. But in reality all access objects in Drupal core are extending AccessResult, which does have this method because it also implements RefinableCacheableDependencyInterface. So in typical Drupal code cache metadata is supported and we can call this method. Drupal is full of these kind of discrepancies, and often developers ignore these cases where a method is part of a different interface.
It is not reliable though. It is always possible someone will implement their own AccessResultInterface object which does not implement RefinableCacheableDependencyInterface and then we get a fatal error. Just putting this simple if statement here makes sure we are never going to throw a fatal error if somebody is using a custom implementation.
We in fact are offering extra functionality (cache metadata support) in case the object we are dealing with supports it. This is commonly called "type widening" in PHP. Modern IDE's like PHPStorm have become very good at pointing out these kind of errors.
I created a PR to document this section: #697
Fixes #675.
This is building on top of #681.