Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
34c64e1
[HttpFoundation] [HttpKernel]
dwgebler Nov 30, 2024
0ccb858
[HttpFoundation]
dwgebler Dec 1, 2024
0c59f2e
[HttpFoundation][HttpKernel]
dwgebler Dec 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Symfony/Component/HttpFoundation/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ CHANGELOG
6.4
---

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* Make `HeaderBag::getDate()`, `Response::getDate()`, `getExpires()` and `getLastModified()` return a `DateTimeImmutable`
* Support root-level `Generator` in `StreamedJsonResponse`
* Add `UriSigner` from the HttpKernel component
Expand Down
9 changes: 9 additions & 0 deletions src/Symfony/Component/HttpFoundation/Request.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1749,6 +1749,15 @@ public function preferSafeContent(): bool
return $this->isSafeContentPreferred = AcceptHeader::fromString($this->headers->get('Prefer'))->has('safe');
}

/**
* Resets the mappings of formats to mime types.
* @internal
*/
public static function resetFormats(): void
{
static::initializeFormats();
}

/*
* The following methods are derived from code of the Zend Framework (1.10dev - 2010-01-24)
*
Expand Down
10 changes: 10 additions & 0 deletions src/Symfony/Component/HttpFoundation/Tests/RequestTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,16 @@ protected function tearDown(): void
Request::setTrustedHosts([]);
}

public function testResetFormats()
{
$request = new Request();
$request->setFormat('json', ['application/problem+json']);
$modifiedRequestFormat = $request->getFormat('application/json');
$this->assertNull($modifiedRequestFormat);
Request::resetFormats();
$this->assertEquals('json', $request->getFormat('application/json'));
}

public function testInitialize()
{
$request = new Request();
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Component/HttpKernel/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* Support backed enums in #[MapQueryParameter]
* `BundleInterface` no longer extends `ContainerAwareInterface`
* Add optional `$className` parameter to `ControllerEvent::getAttributes()`
Expand Down
3 changes: 3 additions & 0 deletions src/Symfony/Component/HttpKernel/Kernel.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,9 @@ public function boot()
{
if (true === $this->booted) {
if (!$this->requestStackSize && $this->resetServices) {
if (method_exists(Request::class, 'resetFormats')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@dwgebler dwgebler Dec 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Request::resetFormats();
}
if ($this->container->has('services_resetter')) {
$this->container->get('services_resetter')->reset();
}
Expand Down
28 changes: 28 additions & 0 deletions src/Symfony/Component/HttpKernel/Tests/KernelTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,34 @@ public function testInitializeContainerClearsOldContainers()
$this->assertFileDoesNotExist($legacyContainerDir.'.legacy');
}

public function testBootResetsRequestFormatsOnReset()
{
$request = new Request();
$request->setFormat('json', ['application/problem+json']);
$modifiedRequestFormat = $request->getFormat('application/json');

$httpKernelMock = $this->getMockBuilder(HttpKernel::class)
->disableOriginalConstructor()
->getMock();

$httpKernelMock
->expects($this->any())
->method('handle')
->with($request);

$kernel = $this->getKernel(['getHttpKernel']);
$kernel->expects($this->any())
->method('getHttpKernel')
->willReturn($httpKernelMock);

$kernel->handle($request);

$kernel->boot();
$kernel->handle($request);
$this->assertNull($modifiedRequestFormat);
$this->assertSame('json', $request->getFormat('application/json'));
}

public function testBootInitializesBundlesAndContainer()
{
$kernel = $this->getKernel(['initializeBundles']);
Expand Down
Loading