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

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

Merged
merged 6 commits into from
Aug 29, 2022

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Aug 27, 2022

The plan is to do a merge up and ignore incoming changes.

Closes #9886

@greg0ire
Copy link
Member Author

I have baselined the following issues:

ERROR: InvalidReturnType - lib/Doctrine/ORM/EntityManager.php:279:116 - The declared return type '(T:fn-doctrine\orm\entitymanagerinterface::find as object)|null' for Doctrine\ORM\EntityManager::find is incorrect, got 'null|object' (see https://psalm.dev/011)
    public function find($className, mixed $id, LockMode|int|null $lockMode = null, int|null $lockVersion = null): object|null


ERROR: InvalidReturnType - lib/Doctrine/ORM/EntityManager.php:378:60 - The declared return type '(T:fn-doctrine\orm\entitymanagerinterface::getreference as object)|null' for Doctrine\ORM\EntityManager::getReference is incorrect, got 'Doctrine\Common\Proxy\Proxy<object>|(T:fn-doctrine\orm\entitymanagerinterface::getreference as object)|null|object' (see https://psalm.dev/011)
    public function getReference(string $entityName, $id): object|null


ERROR: InvalidReturnType - lib/Doctrine/ORM/EntityManager.php:422:75 - The declared return type '(T:fn-doctrine\orm\entitymanagerinterface::getpartialreference as object)|null' for Doctrine\ORM\EntityManager::getPartialReference is incorrect, got 'null|object' (see https://psalm.dev/011)
    public function getPartialReference(string $entityName, $identifier): object|null


ERROR: PossiblyNullArgument - lib/Doctrine/ORM/Query/SqlWalker.php:1183:66 - Argument 1 of Doctrine\ORM\Query\SqlWalker::walkSimpleArithmeticExpression cannot be null, possibly null value provided (see https://psalm.dev/078)
        $sql .= ' ELSE ' . $this->walkSimpleArithmeticExpression($generalCaseExpression->elseScalarExpression) . ' END';


ERROR: PossiblyNullArgument - lib/Doctrine/ORM/Query/SqlWalker.php:1193:62 - Argument 1 of Doctrine\ORM\Query\SqlWalker::walkStateFieldPathExpression cannot be null, possibly null value provided (see https://psalm.dev/078)
        $sql = 'CASE ' . $this->walkStateFieldPathExpression($simpleCaseExpression->caseOperand);


ERROR: PossiblyNullArgument - lib/Doctrine/ORM/Query/SqlWalker.php:1200:66 - Argument 1 of Doctrine\ORM\Query\SqlWalker::walkSimpleArithmeticExpression cannot be null, possibly null value provided (see https://psalm.dev/078)
        $sql .= ' ELSE ' . $this->walkSimpleArithmeticExpression($simpleCaseExpression->elseScalarExpression) . ' END';


ERROR: InvalidFalsableReturnType - lib/Doctrine/ORM/QueryBuilder.php:489:52 - The declared return type 'Doctrine\ORM\Query\Parameter|null' for Doctrine\ORM\QueryBuilder::getParameter does not allow false, but 'Doctrine\ORM\Query\Parameter|false|null' contains false (see https://psalm.dev/143)
    public function getParameter(string|int $key): Parameter|null

* @param ConditionalExpression $conditionalExpression
*/
public function __construct($conditionalExpression)
/** @param ConditionalExpression|ConditionalTerm $conditionalExpression */
Copy link
Member Author

Choose a reason for hiding this comment

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

Allowing A|B in the property but just A with the constructor causes many issues, I don't think it's worth the hassle to keep it.

Copy link
Member

Choose a reason for hiding this comment

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

At least not in this PR. 882 files are quite tiring to read. 😅. New type hint PRs will follow for sure for 3.0 in the future.

@greg0ire greg0ire marked this pull request as ready for review August 27, 2022 20:23
@greg0ire greg0ire changed the title Upgrade to phpcs 3.0 Upgrade to phpcs 10.0 on 3.0.x Aug 27, 2022
@greg0ire greg0ire changed the title Upgrade to phpcs 10.0 on 3.0.x Upgrade to doctrine/coding-standard 10.0 on 3.0.x Aug 27, 2022
composer.json Outdated
@@ -37,7 +37,7 @@
},
"require-dev": {
"doctrine/annotations": "^1.13",
"doctrine/coding-standard": "^9.0.2",
"doctrine/coding-standard": "^9.0.2 || ^10.0",
Copy link
Member

Choose a reason for hiding this comment

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

We can drop 9.0 for ORM 3.0, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

No we can't, otherwise it's not possible to install the project on PHP 7.1 (one more reason to drop it)

Copy link
Member

Choose a reason for hiding this comment

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

ORM 3.0 doesn't support PHP 7 anymore

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh right, sorry, I forgot what PR it was 👍

SenseException
SenseException previously approved these changes Aug 29, 2022
SenseException
SenseException previously approved these changes Aug 29, 2022
- listTableDetails() has been deprecated in favor of introspectTable().
- createSchema() has been deprecated in favor of introspectSchema().
@greg0ire
Copy link
Member Author

Sorry @SenseException , the DBAL methods are removed faster than I'm making changes on this PR!

@SenseException
Copy link
Member

@greg0ire Aren't you doing stuff that's out of scope of a coding standard upgrade?

@greg0ire
Copy link
Member Author

greg0ire commented Aug 29, 2022

I'm doing the strict required minimum to get a green build 😅 , and yes, that involves stuff that does not have to do with CS. We have several jobs that use DBAL 4, so such breakage is frequent and needs to be addressed before it piles up. I've regrouped all of that in the last commit.

@SenseException
Copy link
Member

A separate PR would be preferable than a PR with > 800 changed files 😁 but you already released the ghost from the lamp. 🪔

@greg0ire greg0ire merged commit 52b9653 into doctrine:3.0.x Aug 29, 2022
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.

2 participants