Skip to content

fix(symfony): retain existing associations of formats to MIME types when adding new MIME types #6833

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

Merged
soyuka merged 2 commits into from
Nov 29, 2024
Merged

fix(symfony): retain existing associations of formats to MIME types when adding new MIME types #6833

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
babe3fc
fix(state): add mime types to request formats in content negotiatior
dwgebler Nov 27, 2024
363ad18
fix(state): add mime types to request formats in content negotiator
dwgebler Nov 27, 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
4 changes: 3 additions & 1 deletion src/State/Provider/ContentNegotiationProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,9 @@ public function provide(Operation $operation, array $uriVariables = [], array $c
private function addRequestFormats(Request $request, array $formats): void
{
foreach ($formats as $format => $mimeTypes) {
$request->setFormat($format, (array) $mimeTypes);
$existingMimeTypes = $request->getMimeTypes($format);
$newMimeTypes = array_unique(array_merge((array) $mimeTypes, $existingMimeTypes));
$request->setFormat($format, $newMimeTypes);
Copy link
Member

@soyuka soyuka Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

we think it'd be best to restore Symfony formats to the value it was initially when the request is done (for example in the ResponseProcessor ?). Doing this may allow some unwanted formats in the next request. ping @dunglas

Copy link
Contributor Author

@dwgebler dwgebler Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

we think it'd be best to restore Symfony formats to the value it was initially when the request is done (for example in the ResponseProcessor ?). Doing this may allow some unwanted formats in the next request. ping @dunglas

Yeah I don't know the internals of API Platform well enough to say this fix won't have side-effects on how the system reads MIME types elsewhere, so I can understand the concern. To fix the best way possible I think will unfortunately be a fair bit more effort, because you'll need to read the MIME types for every supported format at the beginning of a request, store that state somewhere, then restore it at the end of every master request. Symfony doesn't provide an interface to iterate or read its internal formats mapping, you can only request it either per-format or per-mimetype on Request's public API.

Edit: having gone through a fair portion of the code today and the test suite, I don't think this change introduces any new problems. API Platform is explicit about injecting the preferred format on every request, so the only issue I can identify is the one this fixes, where request formats are being overwritten in long-running servers rather than expanded. I don't think you will see any benefit to any process of resetting the static Request formats on every request.

This is only a mapping of formats to supported MIME types, and the one(s) you're putting in here will always take precedence over the existing formats, which seems like it should be the behaviour you want. Consider:

array_unique(array_merge(['application/problem+json'],['application/json']));

array(2) {
  [0]=>
  string(24) "application/problem+json"
  [1]=>
  string(16) "application/json"
}

array_unique(array_merge(['application/json'],['application/problem+json', 'application/json']));

array(2) {
  [0]=>
  string(16) "application/json"
  [1]=>
  string(24) "application/problem+json"
}

So you should always be getting the right ordering for the format. You can see this PR hasn't broken your test suite, which is a good sign.

Copy link
Member

@soyuka soyuka Nov 29, 2024

Choose a reason for hiding this comment

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

definitely a good sign as we have some weird tests in there haha

}
}

Expand Down
17 changes: 17 additions & 0 deletions tests/State/Provider/ContentNegotiationProviderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,23 @@ class ContentNegotiationProviderTest extends TestCase
{
use ProphecyTrait;

public function testAddRequestFormatsAddsMimeTypesToFormat(): void
{
$provider = new ContentNegotiationProvider();
$request = new Request();
$formats = ['json' => ['application/problem+json']];

$operation = new Post(outputFormats: $formats);
$context = ['request' => $request];

$provider->provide($operation, [], $context);

$mimeTypes = $request->getMimeTypes('json');

$this->assertContains('application/problem+json', $mimeTypes);
$this->assertContains('application/json', $mimeTypes);
}

public function testRequestWithEmptyContentType(): void
{
$expectedResult = new \stdClass();
Expand Down
Loading