-
Notifications
You must be signed in to change notification settings - Fork 11k
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
[5.4] Fluent partial resource routes #18393
Conversation
registering resources which registeres them on destruction possibly limiting/excluding methods
This relies heavily on GC. Is it guaranteed to run before the actual route is matched? Or before the routes are cached? |
@JosephSilber is correct. This implement is not guaranteed to be correct. |
👎 |
from the collection
allow limiting and excluding routes afterwards
Implemented now in the other way proposed laravel/ideas#412 (comment) Now |
If the PHP manual is correct, then yes the implementation is guranteed to be correct: "The destructor method will be called as soon as there are no other references to a particular object" |
I doubt that is to be taken so literal. There's no way for the runtime to constantly know that there are no longer any references to an object. The only way to know that is at the end of a GC cycle. |
I don't think the PHP manual is correct. It is up to the garbage collector to look for objects with a zero refcount. |
So you're going to submit a PR to the PHP documentation to fix it? It states it very specifically in several places :) |
And if this is the case, should we take ANYTHING in the PHP manual literally? If so, why? |
I'll add my two cents from personal experience, I've had less than stellar results when dealing with destructors. It seemed to be very difficult to rely on when it was run. |
I would actually be very interested if someone can show a situation where it is not run as soon as there are no other references to the object like the documentation states. |
By its very nature, it's non-deterministic, so, it's hard to provide an example and make any guarantees about when garbage collection will take place. |
That said, I was unable to get PHP to call these destructors in an "incorrect" order: https://3v4l.org/qUD51. Maybe there's an ordering by the garbage collector, just due to the implementation specifics? |
There are definitely user error cases I can think of though. Will write an example. |
Here we go: https://3v4l.org/ihpuf. The application runs to completion, before the route gets registered. |
In terms of the PR, I actually already have a big problem with it, the (and if the programmer actually leaves a reference to the returned value somewhere, they are screwed) |
For that reason, I think this should be a 5.5 change, because it changes the contract. |
An implementation to avoid using destructors, would be for the registra to keep a queue of pending routes, and call some resolve method on them all, before we try to match anything. |
Thank you @GrahamCampbell for your time! I implemented this once again based on your suggestion. |
This really was cleaner using destructors tbh, even though some are scared they aren't called when the PHP docs say they are called. Graham's code example is not really reflective of an actual Laravel routes file. |
There's no reason to assume that people do routing in that way. I don't always. |
This solution is robust. And I don't think it's any worse than the destructor way tbh. |
@@ -74,7 +74,7 @@ public function match($methods, $uri, $action); | |||
* @param string $name | |||
* @param string $controller | |||
* @param array $options | |||
* @return void | |||
* @return \Illuminate\Routing\PendingResourceRegistration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this PR should target 5.5, because this is a breaking change to the interface, requiring implementations to return this object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A newbie question: does this require any actions by me? My first PR 🥇
Is this not changing the entire order of when your routes are registered though? Now the resource routes aren't actually registered until a later time and that makes a difference when registering things. |
Also I still haven't seen an actual case that won't work with the destructor approach. 😬 |
Yeh, u have. That example where I showed the variable living beyond the scope we wanted it to. I mean, I could write a laravel app that demonstrates it, but I can't really be arsed. The point is, that if a route remains referenced, after routing happens, then it won't have ever been registered. It's likely people would do this by mistake, but it's very hard for them to debug, if they don't understand the internals. |
The point here is more than that. Even if you don't have a reference to it anymore, there's no guarantee when GC will run.
PHP's docs be damned.
|
While that is true, I haven't been to actually get PHP to not garbage collect in time in my testing to try to break it. |
@GrahamCampbell that's not enough to actually build a feature on top of. |
I agree, that's why I asked for this to be implemented without using that, and it now has been. |
The resources are currently registered only once so calling Should they? See 1048ce7 Pending resources could just be marked as registered and exception thrown if tried to limit/exclude methods afterwards. Registering resources in the RouteCollection each time needed is a little overkill imo. There might e.g. be several |
If you want to re-submit this using the original, simple approach I suggested go for it. I can't really subscribe to the view that the PHP documentation is not to be trusted because then we can't program anything in this language at all to be honest. I will function as if the documentation is true. If it is not, it should be fixed. |
For what it's worth 👎 On the original approach. This new approach is much better IMO. (ultimately, do what taylor says tho, he da boss) |
Discussed in laravel/ideas#412
In 5.4 you can register routes more fluently e.g.
It would be nice to get same behaviour for registering partial resource routes e.g.