Skip to content
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

fix: uri template should respect rfc 6570 #5080

Merged
merged 1 commit into from
Oct 24, 2022

Conversation

soyuka
Copy link
Member

@soyuka soyuka commented Oct 21, 2022

Q A
Branch? 3.0 as a bug fix?
Tickets na
License MIT
Doc PR

Currently we have .{_format} in our URI template which is not valid according to RFC 5670. We should use {._format}.

It breaks operation names how to solve this? Should api-platform generated operation names be concerned by the backward compatibility ? Maybe document that if a user is using an operation name somewhere he should specify the name inside the metadata?

@soyuka soyuka merged commit a4cd12b into api-platform:3.0 Oct 24, 2022
soyuka added a commit that referenced this pull request Nov 4, 2022
* fix: update yaml extractor test file coding standard (#5068)

* fix(graphql): add clearer error message when TwigBundle is disabled but graphQL clients are enabled (#5064)

* fix(metadata): add class key in payload argument resolver (#5067)

* fix: add class key in payload argument resolver

* add null if everything else goes wrong

* fix: upgrade command remove ApiSubresource attribute  (#5049)

Fixes #5038

* fix(doctrine): use abitrary index instead of value (#5079)

* fix: uri template should respect rfc 6570 (#5080)

* fix: remove @internal annotation for Operations (#5089)

See #5084

* fix(metadata): define a name on a single operation (#5090)

fixes #5082

* fix(metadata): deprecate when user decorates in legacy mode (#5091)

fixes #5078

* fix(graphql): use right nested operation (#5102)

* Revert "fix(graphql): use right nested operation (#5102)" (#5111)

This reverts commit 44337dd.

* fix(graphql): always allow to query nested resources (#5112)

* fix(graphql): always allow to query nested resources

* review

Co-authored-by: Alan Poulain <contact@alanpoulain.eu>

* chore: php-cs-fixer update

* fix: only alias if exists for opcache preload

Fixes api-platform/api-platform#2284 (#5110)

Co-authored-by: Liviu Mirea <liviu.mirea@wecodepixels.com>

* chore: php-cs-fixer update (#5118)

* chore: php-cs-fixer update

* chore: php-cs-fixer update

* fix(metadata): upgrade script keep operation name (#5109)

origin: #5105

Co-authored-by: WilliamPeralta <william.peralta18@gmail.com>

* chore: v2.7.3 changelog

* chore: v3.0.3 changelog

Co-authored-by: helyakin <CourcierMarvin@gmail.com>
Co-authored-by: ArnoudThibaut <thibaut.arnoud@gmail.com>
Co-authored-by: davy-beauzil <38990335+davy-beauzil@users.noreply.github.com>
Co-authored-by: Baptiste Leduc <baptiste.leduc@gmail.com>
Co-authored-by: Xavier Laviron <norival@users.noreply.github.com>
Co-authored-by: Alan Poulain <contact@alanpoulain.eu>
Co-authored-by: Liviu Cristian Mirea-Ghiban <contact@liviucmg.com>
Co-authored-by: Liviu Mirea <liviu.mirea@wecodepixels.com>
Co-authored-by: WilliamPeralta <william.peralta18@gmail.com>
@stephanvierkant
Copy link
Contributor

Isn't this a breaking change? I'm using .{_format} in my application and 3.0.3 breaks my application.

@burned42
Copy link

burned42 commented Nov 7, 2022

Same here. It worked on 3.0.2 with the URI templates generated by the upgrade command and 3.0.3 now breaks my application, too.

Replacing .{_format} with {._format} in all the URI templates fixes the issues after upgrading to 3.0.3.

@dannyvw
Copy link
Contributor

dannyvw commented Nov 8, 2022

After installing 3.0.3 our application triggers the php max execution time on this line https://github.com/phpDocumentor/TypeResolver/blob/1.x/src/Types/ContextFactory.php#L189 but not sure if this library has something to do with this PR.

3.0.2 has no issues.

@silverbackdan
Copy link
Contributor

silverbackdan commented Nov 8, 2022

I'm not sure if this commit caused the issue or another somewhere. My application was working on the dev branch fine until 3.0.3 released, then I had to change my uri templates to match.

Eg.

- $newOperation = (new HttpOperation(HttpOperation::METHOD_GET, '/me.{_format}'))
+ $newOperation = (new HttpOperation(HttpOperation::METHOD_GET, '/me{._format}'))

Or XML operation definition:

<operation class="ApiPlatform\Metadata\Patch"
                       name="submit_patch"
                       method="PATCH"
                       - uriTemplate="/forms/{id}/submit.{_format}"
                       + uriTemplate="/forms/{id}/submit{._format}"
                       read="true"
                       deserialize="false"
                       validate="false"

Or I got a 404 not found.

But working on dev until the day of 3.0.3 release or near to it, even after this one was merged, seemed to work fine and tests passed in my application.

@soyuka
Copy link
Member Author

soyuka commented Nov 9, 2022

@dannyvw I don't get it why should this be related?
@everyone else can you explain to me how this breaks? In Symfony routing this should not have any impact I thought I covered the backward compatibility layer here.

@dannyvw
Copy link
Contributor

dannyvw commented Nov 9, 2022

@soyuka Not sure either, but it does something with { and } and it looks like it crashes in some loop. The phpdocumentor library is used as dependency inside Symfony. Will check it later.

@maxhelias
Copy link
Contributor

@dannyvw A max execution time can stop anywhere without being the consequence of this stop. If you have less resources available it will stop early and if you have more it will stop later.
You should rather profile with a tool, like Blackfire or other, your request to know what takes so much time.

@burned42
Copy link

burned42 commented Nov 9, 2022

@soyuka For me the upgrade command converted former Subresource annotations to ApiResource attributes with custom URI templates which included .{_format}. With 3.0.3 those endpoints now just return empty collections and the swagger ui shows _format as a required parameter (but doesn't include it in the request when trying via swagger ui, so it seems to be just some displaying issue maybe).

So e.g. the same GET request to a collection endpoint returns a collection with the correct items with {._format} but does return an empty collection with .{_format} in the URI template.

As said, it's easily fixed by just moving the . one to the right, but I guess it's still a BC break.

edit: to make this clear, there are no error messages or similar, the requests get processed just fine, they just don't produce the expected result anymore.

@dannyvw
Copy link
Contributor

dannyvw commented Nov 9, 2022

@maxhelias I know, but it is only triggered with the 3.0.3 release and always on the same file/line. It is triggered by the metadata system. So maybe it has something to do with this PR. I will check it later, but maybe someone has the same issue. So it was more as information.

soyuka added a commit to soyuka/core that referenced this pull request Nov 15, 2022
@soyuka
Copy link
Member Author

soyuka commented Nov 15, 2022

I'm fixing this thanks for the report and sorry about this breaking change

soyuka added a commit to soyuka/core that referenced this pull request Nov 15, 2022
soyuka added a commit to soyuka/core that referenced this pull request Nov 15, 2022
soyuka added a commit that referenced this pull request Nov 15, 2022
* fix(metadata): _format broken bc promise on #5080

* add deprecation contracts

* fix phpstan
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants