8000 Enable PHPCS rules for PHP 8.0 by derrabus · Pull Request #9293 · doctrine/orm · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Enable PHPCS rules for PHP 8.0 #9293

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
Jan 2, 2022
Merged

Enable PHPCS rules for PHP 8.0 #9293

merged 1 commit into from
Jan 2, 2022

Conversation

derrabus
Copy link
Member
@derrabus derrabus commented Dec 27, 2021 •
edited
Loading

This will add some types, especially to the test suite. Let's see which tests fail and fix the issues on lower branches.

@derrabus derrabus added this to the 3.0.0 milestone Dec 27, 2021
This was referenced Dec 27, 2021
Merged
Merged
@derrabus derrabus force-pushed the improvement/phpcs-80000-rules branch from d636c3a to 3687c3a Compare December 28, 2021 09:50
@derrabus derrabus marked this pull request as ready for review December 28, 2021 09:50
SenseException
SenseException reviewed Dec 28, 2021
View reviewed changes
Copy link
Member
@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

This PR feels like it should be a follow up to a PR that introduces more type hints in all methods.

@derrabus
Copy link
Member Author

Thank you for the review @SenseException. I think, I should've been more clear about how this PR was generated. All I did was raising the PHP language level for CodeSniffer. All other changes are automated with phpcbf. This is how our Doctrine coding standard wants a PHP 8 codebase to look like. 🙂

This PR feels like it should be a follow up to a PR that introduces more type hints in all methods.

Yes, and that's my plan. Applying types everywhere fails miserably (see #9270), mainly because our doc blocks are not as accurate as they need to be for automating that change.

Instead, I try to add types iteratively (see #9292) and if I find inaccurate typings while doing so, I backport the type fixes to 2.11.x (see #9273). Feel free to join that effort. ORM 3.0 is a good oppertunity to apply native types throughout the codebase.

@SenseException
Copy link
Member

I like the change itself because a null value would consistently result in a fatal error in all supported PHP versions. I'm more comfortable when at least the involved methods can have a type hint, because if the phpdocs aren't accurate now, it might be more painful for those who need to migrate. Do I need to review one of the drafts in case of introducing types? Maybe partially introducing types where it works?

@derrabus
Copy link
Member Author

I like the change itself because a null value would consistently result in a fatal error in all supported PHP versions.

"All supported PHP versions" on the 3.0.x branch are 8.0 and 8.1. Given $foo = null, both get_class($foo) and $foo::class would produce a TypeError on these versions. The error message changes, that's all.

Yes, we should get the types straight, but let's do this in follow-ups. This PR sets up our tooling for the new language level. Once it's merged we can work on adding native type declarations where it makes sense.

@derrabus derrabus force-pushed the improvement/phpcs-80000-rules branch 3 times, most recently from 0e8ac8d to 3019422 Compare January 1, 2022 23:34
derrabus
derrabus commented Jan 1, 2022
View reviewed changes
phpcs.xml.dist
@@ -26,6 +26,7 @@
<exclude name="SlevomatCodingStandard.ControlStructures.EarlyExit"/>
<exclude name="SlevomatCodingStandard.Classes.SuperfluousAbstractClassNaming"/>
<exclude name="SlevomatCodingStandard.Classes.SuperfluousExceptionNaming"/>
<exclude name="SlevomatCodingStandard.Classes.ModernClassNameReference.ClassNameReferencedViaFunctionCall"/>
Copy link
Member Author

Choose a reason for hiding this comment

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

I've deactivated the rule for now because the discussion around it seems to block the merge. I can activate it again in a follow-up.

@derrabus derrabus force-pushed the improvement/phpcs-80000-rules branch 2 times, most recently from 1942569 to cfe35ae Compare January 2, 2022 13:56
@SenseException
Copy link
Member

Okay, once the build is green, I'll 👍

@derrabus derrabus force-pushed the improvement/phpcs-80000-rules branch from cfe35ae to 1dfdc1d Compare January 2, 2022 19:43
@derrabus derrabus merged commit 081f3e4 into doctrine:3.0.x Jan 2, 2022
@derrabus derrabus deleted the improvement/phpcs-80000-rules branch January 2, 2022 20:29
@mpdude mpdude mentioned this pull request Jan 17, 2023
Merged
@mpdude mpdude mentioned this pull request Jan 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SenseException SenseException SenseException approved these changes

Assignees
No one assigned
Labels
Improvement
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@derrabus @SenseException
0