- Issue created by @kristiaanvandeneynde
- π¬π§United Kingdom catch
+1. The syntax for
orIf
andandIf
has always seemed overly verbose especially when you try to avoid the situation described above, this would make things much closer to actual PHP operators where code after||
or&&
only runs when necessary. - π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
This will need a bit more love. If we simply use andIfClosure and orIfClosure, we can only avoid calling the closure if the original access result was forbidden. Any other scenario requires comparing the current to the other result, even if we may end up not using the other result. In the current proposal we'd still have to call the other closure to do some inspection.
So perhaps we need something that can signal intent. The example code in the IS could also not call the callable when the original result is N, because it knows the outcome of the closure cannot be F and therefore has no impact.
- π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
Starting to think we need:
- andAllowedIf(\Closure $closure)
- andForbiddenIf(\Closure $closure)
- orAllowedIf(\Closure $closure)
- orForbiddenIf(\Closure $closure)
Perhaps also andAllowedIfHasPermission(string $permission) and orAllowedIfHasPermission(string $permission);
Then the IS would look like:
$result = AccessResult::allowedIfHasPermission($account, 'access user profiles')->andAllowedIf(function($cacheable_metadata) use ($entity) { $cacheable_metadata->addCacheableDependency($entity); return $entity->isActive(); });
All of these would internally do the sanity checks where the other code does not have to run and only then yield to andIf() and orIf() with the result of the callable. This leaves andIf() and orIf() intact whereas the original thought was to get rid of them, so maybe we need to revisit that or make the original methods protected or private.
- Status changed to Needs review
3 months ago 8:22am 10 June 2024 - π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
Pushed a proof of concept. Keep in mind that these helper functions slightly differ from how andIf() and orIf() behave internally.
- In the original logic, if the original result is uncacheable, then the other result is checked for cacheability and merged if it could turn an uncacheable result into a cacheable one.
- In the new logic, we simply do not run the extra code or add the extra cacheability if the original result will not be affected by the closure result.
The whole point was not to run dead code or add dead cacheable metadata. The trade off being that we may end up not trying to make uncacheable results cacheable, which I assume is an edge case that would occur far less than the current situation where cacheable metadata is needlessly added.
- Merge request !8360Proof-of-concept, has slight logic changes compared to orIf() and andIf() β (Open) created by kristiaanvandeneynde
- Status changed to Needs work
3 months ago 5:33pm 17 June 2024 - πΊπΈUnited States smustgrave
Appears to have already received feedback on MR.
- Status changed to Needs review
3 months ago 10:07am 18 June 2024 - πΊπΈUnited States smustgrave
@mxr576 wonder your thoughts on the responses in the threads?
- π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
@smustgrave we had another round of feedback. I am on the fence about two changes (predicate name and a more explicit param documentation.
Once @mxr576 and I agree on the final open issue, we can apply the 3 suggestions and this can be considered accepted and we can add some cases to AccessResultTest
- π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
Okay we are in agreement now. So if a committer could review, we can add tests when the approach is approved.
- π³π±Netherlands Kingdutch
What is the reason we're introducing new methods? Especially since we want to remove
andif
andorIf
. That means we expect all future code to be more verbose and redundant in being named after its argument type (which, unless there's multiple alternatives, is generally considered an anti-pattern).Could we achieve the same thing through a deprecation dance which changes the parameter types but not the method names?
orIf
andandIf
are both already statically typed toAccessResultInterface
so we could first widen them tocallable|AccessResultInterface
, deprecate on aAccessResultInterface
being thrown and then later dropping theAccessResultInterface
type altogether to allow only callables. That leaves the final codebase still with the succinctorIf
andandIf
but with the desired callable type. - π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
I need to update the IS to reflect the latest MR, but the explanation still stands:
AccessResult::andIf() and ::orIf() each have possible combinations that make the passed in access result obsolete. This leads to whatever was used to build the second access result to run for no reason at all. In case of chained calls, it may even lead to the second check's cacheability making it into the result even though the second check never applied.
Could we achieve the same thing through a deprecation dance which changes the parameter types but not the method names?
You mean check if the passed in argument is of type Closure and then run the new code, otherwise run the old code with a deprecation warning? - Status changed to Needs work
about 1 month ago 12:14pm 5 August 2024 - π¬π§United Kingdom catch
That means we expect all future code to be more verbose and redundant in being named after its argument type
This is no longer the case, the methods are ::orAllowedIf() and ::andAllowedIf() in the MR, but the issue summary is out of date.
Moving to needs work for an issue summary update.
- Status changed to Needs review
about 1 month ago 12:24pm 5 August 2024 - π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
Yeah I can see where the confusion is coming from. Wrote a quick update.
- ππΊHungary mxr576 Hungary
Thanks for the IS update!
Remaining tasks
- Introduce new methods
- Deprecate old methods
- Start cleaning up core in a gazillion places
- Make old methods internal
Well, remaining tasks should be scoped to this ticket, isn't it? From 2nd point they are potential follow up tasks to me that can be opened once these new APIs landed.
Speaking of new APIs, probably this should be added to release notes highlights.
- π¦πΊAustralia acbramley
Left a few comments on the... comments π but generally speaking I like this idea a lot.
- π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
Updated the comments, thanks for the review