Automated fixes with phpcbf + manual fixes

[greg0ire]

Blocked by #8150

I often tell people to run vendor/bin/phpcbf to fix their PRs, by habit,
which sometimes results in confusion because it produces a huge unwanted diff.

Also, and this is key, this reduces greatly the diff between branches, which should mean far fewer conflicts when merging up, or when rebasing between 2.7. and 2.8, so maybe this will make @guilhermeblanco 's work easier as per https://github.com/orgs/doctrine/teams/doctrinecore/discussions/14 . Once this is merged, I think we should vow never to change the CS config unless we do it from the lowest branch and merge up.

There are still a lot of cs issues that need to be fixed before we can switch from git-phpcs to just phpcs.

The rules that required the most work are rules about specifying the types of elements instead of just specifying array. I thought that we could ignore it, but then that would mean that it wouldn't be required for new code, which would be bad, so I tried to describe it when I could figure out easily and fast enough, and in other cases, I just used mixed[]. These kind of changes are in the third commit, and if you are unhappy with that, I can drop that commit that has manual fixes. That will still leave us with a vendor/bin/phpcbf that runs without errors.

Next steps, before merging this: creating merge up PRs, so that they can be pushed atomically all at once. That way we have no unknowns about the merge up and how hard they are. I gave merging into 2.8.x a quick try and there are not that many files conflicting.

I still would like a review, or at least comments about the above plan before going ahead, because this took me a lot of time, so I'd like to have everyone on board before I invest even more time on this.

[vv12131415]

If you do it at once, maybe it's better to add more strict arrays than mixed[].
On the other hand, as you mentioned, the purpose of the PR is to fix all the things about phpcbf, and not phpcs/phpstan/psalm. With leaving mixed[] it's also more atomic PR.

[greg0ire]

If you do it at once, maybe it's better to add more strict arrays than mixed[].

It's better for sure, but that would be a lot of work (there are 180 additions of mixed[] in this PR)

[beberlei]

Since this goes into 2.7 i really prefer not do much more work on details. It should be as sage as possible.

I think allowing array is fine, we will catch these things in manual review steps for new code that is added.

[ostrolucky]

The way how Symfony made such massive changes work without fucking up their merge ups is that they merged it to lowest branch (like you are doing), then merging it up, but here is important part: in conflict resolution phase, all conflicting files are reverted. Then, same process is applied for for upper branches. This way, no manual conflict resolution need to be done. It's better explained here symfony/symfony#29623 (comment)

However, in ORM we are (unfortunately) doing rebases of master, not merges and I'm not sure how would that look like there. Since @guilhermeblanco is taking care of 3.x, I am not comfortable approving this without his opinion.

[greg0ire]

For reference, out of 1097 changed files, here are how many files conflict when merging to 2.8.x :

	both modified:   composer.lock
	both modified:   lib/Doctrine/ORM/EntityRepository.php
	both modified:   lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
	both modified:   lib/Doctrine/ORM/Query/AST/Functions/AbsFunction.php
	both modified:   lib/Doctrine/ORM/Query/AST/Functions/CountFunction.php
	both modified:   lib/Doctrine/ORM/Query/AST/Functions/FunctionNode.php
	both modified:   lib/Doctrine/ORM/Query/AST/Functions/SqrtFunction.php
	both modified:   lib/Doctrine/ORM/Query/SqlWalker.php
	both modified:   lib/Doctrine/ORM/UnitOfWork.php
	both modified:   tests/Doctrine/Tests/ORM/Cache/DefaultQueryCacheTest.php
	both modified:   tests/Doctrine/Tests/ORM/Functional/EntityRepositoryTest.php
	both modified:   tests/Doctrine/Tests/ORM/Functional/NewOperatorTest.php
	both modified:   tests/Doctrine/Tests/ORM/Functional/QueryTest.php
	both modified:   tests/Doctrine/Tests/ORM/Functional/SecondLevelCacheQueryCacheTest.php
	both modified:   tests/Doctrine/Tests/ORM/Hydration/ObjectHydratorTest.php
	both modified:   tests/Doctrine/Tests/ORM/Hydration/SimpleObjectHydratorTest.php
	both modified:   tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php
	both modified:   tests/Doctrine/Tests/ORM/Mapping/XmlMappingDriverTest.php
	both modified:   tests/Doctrine/Tests/ORM/Mapping/YamlMappingDriverTest.php
	both modified:   tests/Doctrine/Tests/ORM/Query/QueryTest.php
	both modified:   tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php
	both modified:   tests/Doctrine/Tests/ORM/UnitOfWorkTest.php

For master, I think it will be another story, but in that case we have git merge -s ours like you suggested @ostrolucky

I think getting @guilhermeblanco 's review is indeed very important on this one, let's wait for it.

[greg0ire]

I think I am going to make another PR first, at least for the return types:

[greg0ire]

I think allowing array is fine, we will catch these things in manual review steps for new code that is added.

@beberlei ok so if I understand correctly, you suggest dropping the third commit? Works for me. The main goal of the PR is to have vendor/bin/phpcbf usable, which would make your work on #8147 much more straightforward. It implies merging with a red build, but oh well…

[orklah]

This would be really cool. Working on #8210 triggered dozens of errors in CI, there's so much changes unrelated to the actual PR that the diff won't really be useful at this point.

[greg0ire]

@orklah we've decided to wait until @guilhermeblanco manages to merge up 2.8.x into master before going on with this.

[orklah]

Any news since then?

[greg0ire]

This is now ready for review. Ironically, the only check that does not pass is the CS one, since running phpcbf resulted in many lines being touched, and these lines often contain issues not fixable by phpcbf. The last commit is a cherry-pick of my previous attempt at fixing this issues, alas it was done when we were using phpcs 6 and fixes only 200 or so issues over the 700 issues that remain on the touched lines.

In case you are wondering how many cs issues are left:

A TOTAL OF 3285 ERRORS AND 0 WARNINGS WERE FOUND IN 652 FILES

I might look into automated way of fixing some of those later, but I think there is already great value in this PR as is, since it allows contributors to run vendor/bin/phpcbf to fix their contributions without also getting fixes for other files. I suggest using admin privileges to bypass the cs check.