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

Add support for native PHP8 QueryParam, FileParam and RequestParam attributes #2327

Merged
merged 2 commits into from
Dec 18, 2021

Conversation

W0rma
Copy link
Contributor

@W0rma W0rma commented Aug 19, 2021

Closes #2301

@GuilhemN GuilhemN marked this pull request as ready for review August 26, 2021 07:46
@GuilhemN GuilhemN marked this pull request as draft August 26, 2021 07:46
* @param mixed $default
*/
public function __construct(
string $name = '',
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we add bc layers for these classes, like symfony does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@GuilhemN I guess you refer to the BC layer for the Route annotation.

I'm not sure how such BC layer should look like here.
Before my changes the constructor of the annotations were just empty.

Since all newly added parameters have default values it is still possible to call new QueryParam() and the behavior of this call should not change.

@W0rma W0rma marked this pull request as ready for review November 24, 2021 16:28
@mbabker
Copy link
Contributor

mbabker commented Nov 24, 2021

How about making the annotations reader for the FOS\RestBundle\Request\ParamReader class optional? Right now, the class has a hard dependency on doctrine/annotations and all the Symfony classes made the Reader argument optional when attributes support was added.

@W0rma
Copy link
Contributor Author

W0rma commented Nov 26, 2021

How about making the annotations reader for the FOS\RestBundle\Request\ParamReader class optional?

Good idea. Thank you for the hint! See a0377b2

@diegozavaletatelanto
Copy link

Amazing feature, this will help us, to use PHP Attributes instead of annotations (we need to import the Class, but looks like unused import).
Thanks for your contribution.

@goetas
Copy link
Member

goetas commented Dec 17, 2021

@W0rma do you have anything to add to this PR? I personally think that is fine and I would like to proceed with the merge, what do you think?

@W0rma
Copy link
Contributor Author

W0rma commented Dec 18, 2021

@goetas I tested my branch in a real life project and it worked fine. Therefore, I think it can be merged.

I'm looking forward to the next release with symfony6 and full attribute support :)

@goetas goetas merged commit 519d967 into FriendsOfSymfony:3.x Dec 18, 2021
@goetas goetas added this to the 3.2 milestone Dec 18, 2021
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.

Missing Attribute Annotation
5 participants