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

[PHP 8.3] Add #[\Override] attribute #434

Merged
merged 1 commit into from Jul 28, 2023

Conversation

IonBazan
Copy link
Contributor

@IonBazan IonBazan commented Jul 3, 2023

Adds #[\Override] attribute introduced in PHP 8.3: https://wiki.php.net/rfc/marking_overriden_methods

Failing tests are unrelated

@IonBazan IonBazan changed the title Add #[\Override] attribute [PHP 8.3] Add #[\Override] attribute Jul 3, 2023
@derrabus
Copy link
Member

derrabus commented Jul 8, 2023

LGTM. Would it make sense to add a functional test? Like, using the attribute on a class and verify that we can load it via reflection?

@IonBazan
Copy link
Contributor Author

IonBazan commented Jul 9, 2023

@derrabus we don't usually do that as tests are using the autoloader generated from root composer.json, while symfony/polyfill-php83 dependants will use the sub-split with its own composer.json. I don't think we can test that easily but I agree it is quite prone to error.

@IonBazan
Copy link
Contributor Author

Thanks for the review @nicolas-grekas - addressed the issues.

@nicolas-grekas
Copy link
Member

I'd keep the constructor if it's on the native implem, so that the reflection info is as close as possible to the native one... (can you check this aspect ?)

@nicolas-grekas
Copy link
Member

Thank you @IonBazan.

@nicolas-grekas nicolas-grekas merged commit a4a7235 into symfony:main Jul 28, 2023
6 of 7 checks passed
@IonBazan IonBazan deleted the feature/override branch July 28, 2023 08:45
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

4 participants