Migrate to Doctrine CS 10 on 3.0.x #9886

derrabus · 2022-07-08T10:51:08Z

This is #9866 on 3.0.x. I'd like to keep track of what the new CS standard would do to our precious codebase. 😬

derrabus · 2022-07-08T11:22:41Z

@simPod Apparently, the introduction of the SlevomatCodingStandard.Functions.RequireTrailingCommaInCall.MissingTrailingComma sniff causes some trouble for us. If I run PHPCBF, it fails to fix someof the reported errors. Do you have time to investigate that?

PHPCBF RESULT SUMMARY
--------------------------------------------------------------------------------------------------
FILE                                                                              FIXED  REMAINING
--------------------------------------------------------------------------------------------------
lib/Doctrine/ORM/Tools/Console/Command/ClearCache/ResultCommand.php               FAILED TO FIX
lib/Doctrine/ORM/Tools/Console/Command/SchemaTool/UpdateCommand.php               FAILED TO FIX
lib/Doctrine/ORM/Tools/Console/Command/ClearCache/QueryRegionCommand.php          FAILED TO FIX
lib/Doctrine/ORM/Tools/Console/Command/SchemaTool/CreateCommand.php               FAILED TO FIX
lib/Doctrine/ORM/Tools/Console/Command/SchemaTool/DropCommand.php                 FAILED TO FIX
lib/Doctrine/ORM/Tools/Console/Command/InfoCommand.php                            FAILED TO FIX
lib/Doctrine/ORM/Tools/Console/Command/ClearCache/CollectionRegionCommand.php     FAILED TO FIX
lib/Doctrine/ORM/Tools/Console/Command/MappingDescribeCommand.php                 FAILED TO FIX
lib/Doctrine/ORM/Tools/Console/Command/ClearCache/MetadataCommand.php             FAILED TO FIX
lib/Doctrine/ORM/Tools/Console/Command/ClearCache/EntityRegionCommand.php         FAILED TO FIX
--------------------------------------------------------------------------------------------------
A TOTAL OF 19 ERRORS WERE FIXED IN 10 FILES
--------------------------------------------------------------------------------------------------
PHPCBF FAILED TO FIX 10 FILES
--------------------------------------------------------------------------------------------------

simPod · 2022-07-08T11:24:54Z

@derrabus I think I've already did. Try disabling Generic.Functions.FunctionCallArgumentSpacing.NoSpaceAfterComma. It causes conflict for the fixer.

derrabus · 2022-07-08T11:35:31Z

Indeed, that did the trick, thanks. Is this a bug we need to report to PHPCS or would we rather deactivate that rule in the Doctrine CS?

simPod · 2022-07-08T11:37:11Z

I plan to investigate the conflict and then will see what to do about it. Either report origin lib or/and send PR to Doctrine CS.

derrabus · 2022-07-08T11:47:08Z

tests/Doctrine/Tests/ORM/Functional/OneToOneEagerLoadingTest.php

-        /**
-         * @var Train
-         * @OneToOne(targetEntity="Train", fetch="EAGER")
-         */
-        public $train


This change is a bit unfortunate. SlevomatCodingStandard.TypeHints.ParameterTypeHint.MissingNativeTypeHint is triggered here although it's not just a parameter type that is being added here, but also a property type.

Maybe v8.2 does the trick https://github.com/slevomat/coding-standard/releases/tag/8.2.0

No, these changes have been produced with version 8.2.0 already.

I'm looking at this again and I don't think I understand what the issue is 🤔.

The @var seems redundant so it's added as a native TH.

simPod · 2022-07-08T11:55:22Z

@derrabus the issue has been there for a long time so I don't think it's relevant for our CS v10.

squizlabs/PHP_CodeSniffer#3477

There are two squizlabs sniffs conflicting because they have never understood trailing commas.

derrabus · 2022-07-08T12:03:28Z

Okay, but by introducing more trailing comma rules, we create a footgun if we don't deactivate that conflicting rule in Doctrine CS, don't we?

simPod · 2022-07-08T12:18:37Z

@derrabus both sniffs are useful I think, it will require a fix in squizlabs/PHP_CodeSniffer

morozov · 2022-07-09T20:35:19Z

lib/Doctrine/ORM/AbstractQuery.php

@@ -88,7 +88,7 @@ abstract class AbstractQuery
    /**
     * The user-specified ResultSetMapping to use.
     */
-    protected ?ResultSetMapping $_resultSetMapping = null;
+    protected ResultSetMapping|null $_resultSetMapping = null;


What's the rationale behind enforcing the usage of union types instead of nullable types?

doctrine/coding-standard#266 (comment)

Personally, I believe arbitrary union types (e.g. getValue(sting $key): int|string|false) often reflect poor API design while nullable types are fine (e.g. findUser(int $id): ?User).

I think that enforcing a feature introduced primarily to support the existing poor design of the PHP's own API and the type system as a one-size-fits-all solution is not the right move.

Union types are fine. I must disagree that union types often reflect poor API design. I think the premise is too simplified.

We have two ways of expressing the same thing: a union of a type and null. Normalizing that is reasonable imho. Whether we use the old nullable syntax or the new union syntax does not matter that much to me, as long as we agree on a single way to do it.

And I don't follow that "poor API design" argument either.

All right, so how do you suggest to proceed here?

We do not forbid nullable types in the coding standard: doctrine/coding-standard#266 (comment).

We do not forbid nullable types in the coding standard

That means, we would allow both ways of expressing nullable types? Or would we let the fixer change Foo|null to ?Foo?

Do you want to open a PR on the coding-standards repo that makes the necessary config adjustments? I think, this change should be discussed there.

I think different topic is discussed here. The sniff is not about whether or not it is ok to design API with unions. It's not about API at all.

The sniff is about consistent usage of union types. If there are no unions, the sniff does not apply.

If there's union of Type and null, it should be written as union: Type|null. The archaic ?Type was introduced because of absence of union types in the language. Since there's unions support now, there's no reason to use ? because it's inconsistent. We already have null value and will have null standalone type in 8.2.

That means, we would allow both ways of expressing nullable types? Or would we let the fixer change Foo|null to ?Foo?

If the latter is possible, I would prefer that. Otherwise, the former is fine.

Do you want to open a PR on the coding-standards repo that makes the necessary config adjustments?

No.

derrabus · 2022-08-30T11:40:19Z

Replaced by #10011

derrabus force-pushed the bump/doctrine-cs-on-3.0.x branch from fb41afc to 505b4b4 Compare July 8, 2022 11:18

derrabus force-pushed the bump/doctrine-cs-on-3.0.x branch from 505b4b4 to 6c5a61a Compare July 8, 2022 11:32

derrabus commented Jul 8, 2022

View reviewed changes

morozov reviewed Jul 9, 2022

View reviewed changes

Doctrine CS 10

582d319

derrabus force-pushed the bump/doctrine-cs-on-3.0.x branch from 6c5a61a to 582d319 Compare July 25, 2022 13:59

frankverhoeven mentioned this pull request Jul 28, 2022

chore: Upgrade to Doctrine Coding Standard 10 MyOnlineStore/coding-standard#14

Merged

greg0ire mentioned this pull request Aug 27, 2022

Upgrade to doctrine/coding-standard 10.0 on 3.0.x #10011

Merged

morozov mentioned this pull request Aug 29, 2022

Reconsider enforcement of union types over the nullable ones doctrine/coding-standard#288

Open

derrabus closed this Aug 30, 2022

derrabus deleted the bump/doctrine-cs-on-3.0.x branch August 30, 2022 11:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to Doctrine CS 10 on 3.0.x #9886

Migrate to Doctrine CS 10 on 3.0.x #9886

derrabus commented Jul 8, 2022

derrabus commented Jul 8, 2022

simPod commented Jul 8, 2022 •

edited

derrabus commented Jul 8, 2022

simPod commented Jul 8, 2022

derrabus Jul 8, 2022

simPod Jul 8, 2022

derrabus Jul 8, 2022

simPod Jul 8, 2022

simPod commented Jul 8, 2022

derrabus commented Jul 8, 2022

simPod commented Jul 8, 2022

morozov Jul 9, 2022

simPod Jul 9, 2022

morozov Jul 9, 2022 •

edited

simPod Jul 9, 2022

derrabus Jul 12, 2022

derrabus Jul 12, 2022

morozov Jul 12, 2022

derrabus Jul 12, 2022

simPod Jul 12, 2022

morozov Jul 12, 2022

derrabus commented Aug 30, 2022

Migrate to Doctrine CS 10 on 3.0.x #9886

Migrate to Doctrine CS 10 on 3.0.x #9886

Conversation

derrabus commented Jul 8, 2022

derrabus commented Jul 8, 2022

simPod commented Jul 8, 2022 • edited

derrabus commented Jul 8, 2022

simPod commented Jul 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simPod commented Jul 8, 2022

derrabus commented Jul 8, 2022

simPod commented Jul 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morozov Jul 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derrabus commented Aug 30, 2022

simPod commented Jul 8, 2022 •

edited

morozov Jul 9, 2022 •

edited