[HttpFoundation][HttpKernel] Reset request formats after each main request #59053

dwgebler · 2024-12-01T13:34:15Z

Add public static method to reset Request format to mime mappings Reset Request format to mime mappings on Kernel boot for each new main request

Q	A
Branch?	6.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues	Fix #59036
License	MIT

Adds a new public static method to Request to reset the mappings of formats to MIME types. Adds a call to this method just before resetting services in Kernel::boot().

Add public static method to reset Request format to mime mappings Reset Request format to mime mappings on Kernel boot for each new main request

Fix typo in docblock comment Initialize $formats in reset function

fabpot · 2024-12-06T13:44:08Z

This is a new feature, so it should be on 7.3 instead.
I understand this could be considered as a fix but not if we need to add new methods.

dwgebler · 2024-12-06T14:01:14Z

This is a new feature, so it should be on 7.3 instead. I understand this could be considered as a fix but not if we need to add new methods.

@fabpot see #59109 CC @dunglas @soyuka

src/Symfony/Component/HttpFoundation/Request.php

src/Symfony/Component/HttpKernel/Kernel.php

soyuka

We discussed this at the SymfonyCon and thought best to put @internal on the method. When merging into 7.3 the @internal flag will be removed.

dwgebler · 2024-12-06T15:28:42Z

We discussed this at the SymfonyCon and thought best to put @internal on the method. When merging into 7.3 the @internal flag will be removed.

Was @fabpot part of this discussion? Because I'm confused as to whether this can go ahead for merge into 6.4 now, even with an internal flag...whose decision takes precedence here?

dunglas · 2024-12-06T15:38:19Z

@dwgebler yes Fabien was part of the discussion. This can go ahead for merge in 6.4 with changes requested by @soyuka.

Mark Request::resetFormats as internal for 6.4

dwgebler · 2024-12-06T15:43:27Z

@dwgebler yes Fabien was part of the discussion. This can go ahead for merge in 6.4 with changes requested by @soyuka.

@dunglas thanks for clarifying, I've updated this branch to add internal flag. Would've loved to have been at SymfonyCon, hopeful I can make next year!

fabpot · 2024-12-07T08:13:02Z

src/Symfony/Component/HttpFoundation/CHANGELOG.md

@@ -4,6 +4,7 @@ CHANGELOG
 6.4
 ---

+ * Add `Request::resetFormats()` internal method to reset the list of supported format to MIME type mappings


This should be removed as bug fixes are not listed in the CHANGELOGs.

fabpot · 2024-12-07T08:13:11Z

src/Symfony/Component/HttpKernel/CHANGELOG.md

@@ -4,6 +4,7 @@ CHANGELOG
 6.4
 ---

+ * Add call to `Request::resetFormats()` in `Kernel::boot()` to reset the request formats at the start of a new main request.


This should be removed as bug fixes are not listed in the CHANGELOGs.

fabpot · 2024-12-07T08:15:28Z

src/Symfony/Component/HttpKernel/Kernel.php

@@ -110,6 +110,9 @@ public function boot()
    {
        if (true === $this->booted) {
            if (!$this->requestStackSize && $this->resetServices) {
+                if (method_exists(Request::class, 'resetFormats')) {


I don't like that this leaks in the Kernel.
Couldn't we move this to RequestStack and implement the ResetInterface there? (we might have discussed it in Vienna but I'm not anymore on the conclusion on this possibility)

Indeed having everything in the same component is better.
Tools that doesn't use RequestStack (as we discussed) don't use HttpKernel either anyway.

That's adding a new public method to RequestStack, so should that be marked as @internal to go in 6.4?

I wonder if that's necessary or if we couldn't handle it inside RequestStack::push() when the stack is empty?

@fabpot you know I'm not sure I love implementing ResetInterface on RequestStack. Consider it'll look like:

/** * Resets stateful request properties. * @return void * @internal */ public function reset(): void { Request::resetFormats(); }

And yet, is this what you would expect "resetting" the request stack to do? It doesn't seem like it semantically makes sense in context of where it's placed.

Likewise I wouldn't put it in push() with a check for an empty stack, because it seems like it's polluting the behaviour of push and violating single responsibility.

Up to you (obviously) but the HttpKernel component seems like the thing which should be responsible for doing this to me. Can we not just merge the existing approach with @internal and get rid of the method_exists call? It's not as if Kernel doesn't already rely on certain things existing in the Request class; it's not a decoupled component.

We have to keep the method_exists check because versions can be mixed. HttpKernel can be used with an older version of HttpFoundation.

Otherwise, both approaches work for me.

Fair enough in respect of method_exists but would be good to get an answer from Fabien on the main question around this solution versus RequestStack. Use of the ResetInterface doesn't feel very intuitive to me there as per my other comment but I'm happy to go with whatever someone is willing to accept for merge.

nicolas-grekas · 2025-01-07T17:22:23Z

Thanks for proposing. After reading the discussion, I opened #59403 with an approach centered on HttpFoundation and service resetting.

dwgebler · 2025-01-07T18:15:40Z

Thanks for proposing. After reading the discussion, I opened #59403 with an approach centered on HttpFoundation and service resetting.

When I discovered the buggy behaviour in API Platform running on the FrankenPHP runtime, I put quite a lot of time into raising the issue, documenting it, providing a way to reproduce it, identifying the cause, back and forth with Kevin and other contributors to both API Platform and Symfony, creating this PR, getting told make it a new feature against 7.x, no, now we want it to be a bugfix against 6.x, bike-shedding about the details of where the change belongs in the components, etc.

Open source contributions matter and yes, this change may be a very small thing but the bug it caused isn't and I feel like it would have been more appropriate to simply ask me to make any changes in my PR that you as the official maintainers wanted, because by closing my PR, my GitHub account is then not an upstream contributor, despite the time I've put in here to find and fix something that needed fixing, because it will impact people.

I'm not trying to be petty here, but please understand when I go through that process, only for one of the core team to go "Thanks, we're going to take your fix and implement it just slightly differently under our own accounts", it doesn't make me want to jump through all those hoops again should I ever discover any other bugs or develop features that might be useful upstream in my own projects in future.

nicolas-grekas · 2025-01-08T08:41:01Z

Hi @dwgebler, I'm sorry you feel that way. I've also been involved in this issue since a few weeks, talking about it here on github but also at conferences in Paris and Vienna with Antoine, Kevin, Fabien at least. When I reopened your PR yesterday, I saw that many alternative proposals where made but not explored since 2 weeks so I went ahead and opened my IDE to see how they could materialize. I would have suggested some changes if the patch I ended up with wasn't completely different from the approach here, and from any other suggested approaches actually (not totally alien of course). Thank you for digging this topic, that helped make Symfony better for sure. I hope another PR of yours will make it into core.

dwgebler · 2025-01-08T10:26:39Z

@nicolas-grekas I'm glad there is a fix going in, somewhere, because that is the most important thing. I remain deeply grateful to everyone involved with Symfony and API Platform for creating some of the best products in the PHP ecosystem and giving them away for free, and I mean this with the utmost sincerity. Maybe I am being petty, I don't know, I don't intend to be; it may be the tiniest of code changes, it's just a fair amount of time and back and forth with a few people went into it (hence the conversation I'm told it generated between you at SymfonyCon about the best place to fix), getting the PR aligned with that, ensuring everything conformed to all the project's contribution guidelines etc. and rightly or wrongly, it just left me feeling like what's the point of doing all that if in the end, next time I might as well just file a bug report and leave it at that. It's not about me wanting some sort of special recognition for authoring a tiny fix, it's that it puts up barriers to contributing and being a contributor, you know - it leaves me thinking if the process is that much over something this small, what's it like for anything more significant? So I hope that explains where I'm coming from.

dwgebler added 2 commits December 1, 2024 13:08

[HttpFoundation] [HttpKernel]

34c64e1

Add public static method to reset Request format to mime mappings Reset Request format to mime mappings on Kernel boot for each new main request

[HttpFoundation]

0ccb858

Fix typo in docblock comment Initialize $formats in reset function

carsonbot added Status: Needs Review Bug HttpFoundation HttpKernel labels Dec 1, 2024

dwgebler mentioned this pull request Dec 1, 2024

[HttpFoundation][HttpKernel] Reset request formats after each main request #59047

Closed

dunglas approved these changes Dec 1, 2024

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Dec 1, 2024

soyuka approved these changes Dec 3, 2024

View reviewed changes

soyuka reviewed Dec 6, 2024

View reviewed changes

src/Symfony/Component/HttpFoundation/Request.php Show resolved Hide resolved

src/Symfony/Component/HttpKernel/Kernel.php Outdated Show resolved Hide resolved

soyuka reviewed Dec 6, 2024

View reviewed changes

[HttpFoundation][HttpKernel]

0c59f2e

Mark Request::resetFormats as internal for 6.4

fabpot requested changes Dec 7, 2024

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Dec 7, 2024

fabpot reviewed Dec 7, 2024

View reviewed changes

norkunas mentioned this pull request Dec 22, 2024

Request format being overwritten to application/problem+json api-platform/core#6885

Closed

nicolas-grekas closed this Jan 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HttpFoundation][HttpKernel] Reset request formats after each main request #59053

[HttpFoundation][HttpKernel] Reset request formats after each main request #59053

dwgebler commented Dec 1, 2024

fabpot commented Dec 6, 2024

dwgebler commented Dec 6, 2024 •

edited

Loading

soyuka left a comment

dwgebler commented Dec 6, 2024

dunglas commented Dec 6, 2024 •

edited

Loading

dwgebler commented Dec 6, 2024

fabpot Dec 7, 2024

fabpot Dec 7, 2024

fabpot Dec 7, 2024

dunglas Dec 7, 2024

dwgebler Dec 7, 2024

xabbuh Dec 7, 2024

dwgebler Dec 7, 2024 •

edited

Loading

dunglas Dec 22, 2024

dwgebler Dec 23, 2024

nicolas-grekas commented Jan 7, 2025

dwgebler commented Jan 7, 2025

nicolas-grekas commented Jan 8, 2025 •

edited

Loading

dwgebler commented Jan 8, 2025 •

edited

Loading

[HttpFoundation][HttpKernel] Reset request formats after each main request #59053

[HttpFoundation][HttpKernel] Reset request formats after each main request #59053

Conversation

dwgebler commented Dec 1, 2024

fabpot commented Dec 6, 2024

dwgebler commented Dec 6, 2024 • edited Loading

soyuka left a comment

Choose a reason for hiding this comment

dwgebler commented Dec 6, 2024

dunglas commented Dec 6, 2024 • edited Loading

dwgebler commented Dec 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwgebler Dec 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Jan 7, 2025

dwgebler commented Jan 7, 2025

nicolas-grekas commented Jan 8, 2025 • edited Loading

dwgebler commented Jan 8, 2025 • edited Loading

dwgebler commented Dec 6, 2024 •

edited

Loading

dunglas commented Dec 6, 2024 •

edited

Loading

dwgebler Dec 7, 2024 •

edited

Loading

nicolas-grekas commented Jan 8, 2025 •

edited

Loading

dwgebler commented Jan 8, 2025 •

edited

Loading