Skip to content

[DependencyInjection] Add the Required attribute #37545

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

Jump to bottom
Merged
merged 1 commit into from
Sep 7, 2020
Merged

[DependencyInjection] Add the Required attribute #37545

merged 1 commit into from
Sep 7, 2020

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented Jul 9, 2020 •
edited
Loading

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets N/A
License MIT
Doc PR TODO

This PR proposes a new attribute #[Required] that can be used instead of the @required annotation.

@derrabus derrabus force-pushed the improvement/required-attribute branch from 09e3ba2 to caf5ba7 Compare July 9, 2020 22:13
@nicolas-grekas nicolas-grekas added this to the next milestone Jul 12, 2020
@derrabus derrabus force-pushed the improvement/required-attribute branch from caf5ba7 to 64767ef Compare July 12, 2020 19:26
@derrabus derrabus force-pushed the improvement/required-attribute branch from 64767ef to c166a80 Compare August 8, 2020 13:42
@derrabus
Copy link
Member Author

derrabus commented Aug 8, 2020 •
edited
Loading

The PR has been updated to match the new attribute syntax and is now compatible with php 8.0.0-beta1.

@derrabus derrabus force-pushed the improvement/required-attribute branch from c166a80 to 91f0692 Compare September 3, 2020 09:12
@derrabus
Copy link
Member Author

derrabus commented Sep 3, 2020

The PR has once again been updated because of a syntax change: php/php-src@8b37c1e

nicolas-grekas
nicolas-grekas reviewed Sep 3, 2020
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

thanks, looks nice, here are some minor comments
btw, if you want to have a look at making the PHP8 build green (or just have less failures), that'd be awesome :)

@derrabus derrabus force-pushed the improvement/required-attribute branch from 91f0692 to 9f7f27f Compare September 3, 2020 20:35
@nicolas-grekas
Copy link
Member

The failure on travis can be skipped by listing the fixture file in .github/patch-types.php maybe?

@derrabus derrabus force-pushed the improvement/required-attribute branch from 9f7f27f to ea26244 Compare September 7, 2020 11:33
@derrabus
Copy link
Member Author

derrabus commented Sep 7, 2020

I've added the file there. 👍

@fabpot
Copy link
Member

fabpot commented Sep 7, 2020

Thank you @derrabus.

@fabpot fabpot mentioned this pull request Sep 7, 2020
Closed
@fabpot fabpot merged commit b26f11c into symfony:master Sep 7, 2020
@derrabus derrabus deleted the improvement/required-attribute branch September 7, 2020 12:13
@derrabus derrabus mentioned this pull request Sep 18, 2020
Merged
nicolas-grekas added a commit that referenced this pull request Sep 18, 2020
@nicolas-grekas
minor #38232 Added missing changelog entries (derrabus)
163a2ab 
This PR was merged into the 5.2-dev branch.

Discussion
----------

Added missing changelog entries

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | #37545 #37474
| License       | MIT
| Doc PR        | N/A

I've added the missing changelog entries for the two tickets above.

Commits
-------

14642fb Added missing changelog entries.
@TomasVotruba
Copy link
Contributor

This change is now covered in @rectorphp https://github.com/rectorphp/rector/pull/4348/files#diff-8e9faa1ff0eb70ba27a9918bf9b062b4

Let me know if we got it right 😉

@derrabus
Copy link
Member Author

derrabus commented Oct 4, 2020

@TomasVotruba Your fixture looks correct. In addition to the case that it covers, the @required annotation could also be attached to a public typed property.

@TomasVotruba
Copy link
Contributor

TomasVotruba commented Oct 4, 2020 •
edited
Loading

@derrabus Thanks for feedback 👍

It was not clear from the tests, but property is supported as well.
I've added the test fixture to have property covered explicitly too:
https://github.com/rectorphp/rector/pull/4353/files

@nicolas-grekas nicolas-grekas removed this from the next milestone Oct 5, 2020
@nicolas-grekas nicolas-grekas added this to the 5.2 milestone Oct 5, 2020
@fabpot fabpot mentioned this pull request Oct 5, 2020
Merged
@derrabus derrabus mentioned this pull request Oct 12, 2020
Merged
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Oct 17, 2020
@javiereguiluz
minor #14394 [DependencyInjection] Document the Required attribute (d…
95b8a38 
…errabus)

This PR was merged into the 5.x branch.

Discussion
----------

[DependencyInjection] Document the Required attribute

This PR adds documentation for symfony/symfony#37545 and fixes #14189.

Commits
-------

701f704 [DependencyInjection] Document the Required attribute.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
DependencyInjection Feature Status: Reviewed
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.
5 participants
@derrabus @nicolas-grekas @fabpot @TomasVotruba @carsonbot