-
Notifications
You must be signed in to change notification settings - Fork 11.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[11.x] without_recursion() helper #52865
Conversation
Thanks for submitting a PR! Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface. Pull requests that are abandoned in draft may be closed due to inactivity. |
09d4fa4
to
9d0a1fa
Compare
Deprecated `$model->withoutRecursion()` in favour of the more generalized global `withoust_recursion()` helper.
9d0a1fa
to
35ae45c
Compare
Hi, does it have the same issue as PreventsCircularRecursion? #52727 |
@sanfair From a quick read at the code, it shouldn't have this issue |
It shouldn't do because it's not a part of the model any longer. It's using an external class to keep track of recursion which means that you shouldn't have issues with mocked classes. |
Couldn't we think of this as a support class rather than a function-based helper? use Illuminate\Support\Recursion;
Recursion::run(fn () => /* ... */); // based on a prior check to execute or not... Maybe with this approach, we can even think about other things: use Illuminate\Support\Recursion;
Recursion::allowed(fn () => /* ... */);
Recursion::unallowed(fn () => /* ... */);
Recursion::alwaysAllowed();
Recursion::alwaysUnallowed(); |
What would |
If you think about it for the framework, okay, I agree with you. Now, if you think of it offering as a feature to the dev, well... you never know! |
If you want to access the classes that are doing the work, you can. The global helper mostly serves the purpose of making it easier to access, and providing the best frame of reference for the You could always, if you really wanted to, do it manually: use Illuminate\Support\Recursable;
use Illuminate\Support\Recurser;
// ...
Recurser::instance()->withoutRecursion(new Recursable(
fn () => /* ... */,
null,
$this,
'some key',
));
I'm honestly not sure what those would do... This change is explicitly to guard against infinite recursion - something that is 100% a bug, without question, every single time that it happens. This isn't a function that I expect that many people will reach for often, but when they need it, it'll be helpful. |
To reduce code for `: void` returning functions
Co-authored-by: Adrien Foulon <[email protected]>
I think I'll leave this one for a package implementation for now. 👍 |
I was super excited about this getting merged. Can we at least get the #52660 issue with |
@AndrewMast I've pulled out just those fixes into #52883 |
In case anyone was actually interested in this, I've pulled the core concept out into a standalone package: It's more or less the same thing, but built to have no dependencies. It's very unlikely that anyone will ever actually need this in standard code, but hey - it exists now. |
Not too long ago, to get
chaperone()
/inverse()
through, I added thePreventsCircularRecursion
trait for Eloquent in #52461 to better handle serializing models that have circular references. There was some discussion on that PR between @rodrigopedra and @Tofandel where it was questioned why I was using a staticWeakMap
instead of just a property on the object, and some of the other decisions made there.I made those decisions because I had never planned this functionality to be tied solely to Eloquent.
Introducing
without_recursion()
This PR adds a new global helper function that can be used to prevent recusion of any function, not just functions inside of Models. This is done by using two new items added into the
Illuminate\Support\
namespace:Illuminate\Support\Recursable
- a class that is similar toIlluminate\Support\Onceable
, but is better suited to the needs of tracking and preventing recursive calls.Illuminate\Support\Recurser
- a class that serves a similar role toIlluminate\Support\Once
by tracking recursive function calls against individual instances, and handling returning alternate values as required.These have a very small
public
interface because you're not meant to interact with them directly (although you totally could if you wanted). The intention is that you only interact with them via the new support helper functionwithout_recursion()
.Stop blabbing, and show me how to use it, Sam.
Sure! It's super simple!
That's an odd example, but you can see that every time
some_recursive_function()
is called insidewithout_recursion()
, it returns the second value - the callback isn't called again, but as soon as the current call stack finishes, that stored value is forgotten and the callback will get called again.How about use in a class?
Classes are where
without_recursion()
shines:As you can see,
without_recursion()
is tied to the specific object that called it automatically, so other objects from the same class don't get affected. This is linked to the file where the call was made, the class, and the function. Unlikeonce()
it doesn't take into account the line thatwithout_recursion()
was called from unless it's a function in the global scope.The value that I want to return on recursion is expensive to calculate
That's fine! If you pass a callable for the second parameter, it won't actually be called until the function recurses, and the result will be cached in its place:
In this example, we don't want to do the expensive calculation until the path loops back on itself, and we don't want to do it again if it loops back again from somewhere else. Passing a closure to
$onRecursion
will only resolve when the same function is called on the same object in the same call tree. If the function is called a third time, the result of the closure is provided without calling the closure again.Specifying the recursion key and object
When used in its most basic form,
without_recursion()
will generate a unique key based the filename, class, and function that called it. The key for theLinkedList
example above would be something like/path/to/LinkedList.php:LinkedList@children
. ForModel::toArray()
it would be/path/to/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php:Illuminate\Database\Eloquent\Model@toArray
. If the caller isn't in a class, then the key will be something like/path/to/file.php:functionName
or if there's no function name .0it will fall back to the line where it was called:/path/to/file.php:123
Each key is stored against the object itself (if the call was made from an instantiated class), so each object will have it's own stack of recursive calls without interfering with eachother.
If, for whatever reason, you need to specify a different key, you can specify it with the third parameter,
as: 'your key'
Note that if you specify the key it won't automatically attach to an object, so you can also specify the object with the fourth parameter,
for: $object
Or you can always just scope the method to any other object:
Advanced usage
You can make use of the callable
$onRecursion
value to do fun things like dispatching a job if an object appears more than once in a tree without dispatching several jobs for the same object:Or you can return a callable from your callable, so that you could log the number of duplicates in a tree:
Other Changes
Model methods migrated
I didn't think that it was right to add this new global function, and leave Model out, so I've updated everything that relied on
PreventsCircularRecursion
to just usewithout_recursion()
now.PreventsCircularRecursion
deprecatedIt hasn't been in the wild for long, but I've left
PreventsCircularRecursion
itself alone (aside from marking it as@deprecated
) just in case somebody actually used it. I probably should have put this effort in originally (and it was half-done, I just wanted to get the PR up and out). This method is better.touchesParents()
is now coveredIn #52660, @AndrewMast mentioned that showed up when you had
$touches = ['parent']
on a chaperone'd child model. I've included a fix for this too.Final notes
I would have done this without using
static
instances, instead using the application/a service provider, but I didn't feel like this really "belonged" inFoundation
, so I took the lead fromOnce
/Onceable
and put it intoSupport
without any reliance on the service container. There's not really any guidance here, so I just played it by ear.