-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[RequestStack] Convert $requests
property from private to protected
#49554
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
Comments
And here is my PR to change this: #49556 |
I don't think we want to make it private. The request property is a source of truth and should stay immutable. If you want to get the main request you have the getMainRequest function. What exactly is your need? |
Changing properties to protected increases the maintenance cost a lot (because protected properties are covered by the BC promise). |
I need to extend the original |
"I need to extend the class" is not a use case. It is a solution that implies making maintenance harder. We need the use case, so that we can think about potential alternative solutions that would not require such cost. |
Thus, allowing child classes to alter the stack of requests directly might lead to weird bugs when that interacts with the logic in HttpKernel that maintains that stack for handled requests. |
while ($requestStack->pop());
If this is a thing, we could add an accessor for that or let you iterate over the requests. Can you elaborate why you need to access a request that is neither the current nor its parent nor the main one? |
Note that by clearing all requests, you will probably break the integration with the HttpKernel (and cause weird bugs in the profiler for instance) as the stack would become out of sync with requests that are actually handled by the kernel. So clearly, this misses an explanation about why you want this clearing. |
In the middleware function need to scan all previous requests and filter out some of them by the business rules, or clean all of them and insert a new "correct" one. Yes, this looks is a very specific logic for my individual case, and changing But if this change can cause a lot of other regressions, let's keep it as is, maybe? And for my case, I can resolve the problem by copy-pasting the whole RequestStack class to my own one, and changing it, instead of extending the original one. Yes, that looks bad but seems now it's the only way. |
I fail to see what that means. The requests in the stack correspond to nested requests (when a request triggers a subrequest that triggers another subrequest). They are not previous requests. And this inserting of a "correct" one is probably flawed as well, as the HttpKernel would still be dealing with the request it received. |
Okay, this is my very specific case, so let's close the discussion. |
Description
In the RequestStack.php the property
$requests
is marked as private, which makes it impossible to operate with it in children classes.Will be good to change it to
protected
to allow interaction with it in children's classes. For example, for my project, I need an additional function to clear (reset) the list of all requests, without popping-out one by one viapop()
, and can't do it now via extending Symfony's RequestStack, because of the property is private.Example
No response
The text was updated successfully, but these errors were encountered: