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

Get Psalm job to pass with DBAL 3 #8884

Closed
greg0ire opened this issue Aug 5, 2021 · 6 comments · Fixed by #8915 or #8897
Closed

Get Psalm job to pass with DBAL 3 #8884

greg0ire opened this issue Aug 5, 2021 · 6 comments · Fixed by #8915 or #8897

Comments

@greg0ire
Copy link
Member

greg0ire commented Aug 5, 2021

Recently, a CI job was added on the 2.10.x branch to statically analyze this project after forcing the installation of doctrine/dbal v3.

In order not to disrupt contributors, we made sure it would not fail the build although it really does not pass at all:

continue-on-error: "${{ matrix.status == 'experimental' }}"

If you want to help us fix it or get closer to fixing it, you can reproduce the failures locally by running (from the 2.10.x branch)

composer require doctrine/dbal ^3.0
vendor/bin/psalm

When this issue is fixed, we should create a new one about making this job no longer experimental.

@greg0ire greg0ire changed the title Get Psalm job to pass Get Psalm job to pass with DBAL 3 Aug 5, 2021
@scyzoryck
Copy link
Contributor

One of the loudest issue are changes introduced in doctrine/dbal@5d03f6e - it affects Doctrine\ORM\Configuration class. I wonder if it would be better to switch to the dedicated property per each config value, or stays with $_attributes property to keep backward compatibility with classes that might extend Doctrine\ORM\Configuration.

@greg0ire
Copy link
Member Author

greg0ire commented Aug 5, 2021

Maybe re-declaring the property in the ORM class would solve the issue. Switching to a dedicated property per config value in the ORM class is a separate thing IMO, it can be done fully independently I think.

scyzoryck pushed a commit to scyzoryck/orm that referenced this issue Aug 5, 2021
…l to meet changes in doctrine/dbal v3.

Fix psalm issues with type: `PossiblyNullArgument`, found after updating doctrine/dbal to v3. Override `null` passed as offset with `0` in calls to `Doctrine\DBAL\Platforms\AbstractPlatform::modifyLimitQuery`. Override `null` passed as lockMode with `LockMode::NONE` in calls to `Doctrine\DBAL\Platforms\AbstractPlatform::appendLockHint`.
Partialy fixes doctrine#8884
@scyzoryck
Copy link
Contributor

The last type of errors seems to be connected with things that has been deprecated in doctrine/dbal 2.x and removed in 3.x, for example: doctrine/dbal#3211 . Do you think that it will be fine to thrown exception in that case if somebody is trying to use such functionalities with doctrine/orm:2.10.x and doctrine/dbal:3.x? It looks like all of such cases are marked as deprecated in doctrine/orm:2.10.x.

@greg0ire
Copy link
Member Author

greg0ire commented Aug 7, 2021

Do you have an example?

@scyzoryck
Copy link
Contributor

Sure.
UuidGenerator is using getGuidExpression function that has been removed in dbal 3.x doctrine/dbal#3211

$sql = 'SELECT ' . $conn->getDatabasePlatform()->getGuidExpression();

There is no internal replacement for this method - just suggested to use ramsey/uuid instead.

I can try to split it by case and create separate PRs for each issue :)

@greg0ire
Copy link
Member Author

greg0ire commented Aug 7, 2021

Oh right, I even deprecated this in #8813 . If we want to have DBAL v3 with ORM v2, then I think we should indeed throw an exception letting people know they need to run composer require doctrine/dbal ^2.0 if they wish to continue using this feature.

I can try to split it by case and create separate PRs for each issue

That would be a great approach 👍

scyzoryck pushed a commit to scyzoryck/orm that referenced this issue Aug 8, 2021
…een removed in doctrine/dbal:3.x

To keep backward compatibility we need to redeclare this property to keep using it.
Partially fixes doctrine#8884
scyzoryck pushed a commit to scyzoryck/orm that referenced this issue Aug 8, 2021
…een removed in doctrine/dbal:3.x

To keep backward compatibility we need to redeclare this property to keep using it.
Partially fixes doctrine#8884
scyzoryck pushed a commit to scyzoryck/orm that referenced this issue Aug 8, 2021
…een removed in doctrine/dbal:3.x

To keep backward compatibility we need to redeclare this property to keep using it.
Partially fixes doctrine#8884
scyzoryck pushed a commit to scyzoryck/orm that referenced this issue Aug 8, 2021
…ine/dbal:3.x.

Generating `getGuidExpression` has been removed in doctrine/dbal:3.x.

Partially fixes doctrine#8884
scyzoryck pushed a commit to scyzoryck/orm that referenced this issue Aug 10, 2021
…ine/dbal:3.x.

Generating `getGuidExpression` has been removed in doctrine/dbal:3.x.

Partially fixes doctrine#8884
scyzoryck pushed a commit to scyzoryck/orm that referenced this issue Aug 10, 2021
…ine/dbal:3.x.

Generating `getGuidExpression` has been removed in doctrine/dbal:3.x.

Partially fixes doctrine#8884
scyzoryck pushed a commit to scyzoryck/orm that referenced this issue Aug 10, 2021
scyzoryck pushed a commit to scyzoryck/orm that referenced this issue Aug 10, 2021
scyzoryck pushed a commit to scyzoryck/orm that referenced this issue Aug 15, 2021
…ine/dbal:3.x.

Generating `getGuidExpression` has been removed in doctrine/dbal:3.x.

Partially fixes doctrine#8884
scyzoryck pushed a commit to scyzoryck/orm that referenced this issue Aug 15, 2021
…ine/dbal:3.x.

Generating `getGuidExpression` has been removed in doctrine/dbal:3.x.

Partially fixes doctrine#8884
scyzoryck pushed a commit to scyzoryck/orm that referenced this issue Aug 15, 2021
scyzoryck pushed a commit to scyzoryck/orm that referenced this issue Aug 15, 2021
…ine/dbal:3.x.

Generating `getGuidExpression` has been removed in doctrine/dbal:3.x.

Partially fixes doctrine#8884
scyzoryck pushed a commit to scyzoryck/orm that referenced this issue Aug 15, 2021
…ine/dbal:3.x.

Generating `getGuidExpression` has been removed in doctrine/dbal:3.x.

Partially fixes doctrine#8884
scyzoryck pushed a commit to scyzoryck/orm that referenced this issue Aug 16, 2021
…ine/dbal:3.x.

Generating `getGuidExpression` has been removed in doctrine/dbal:3.x.

Partially fixes doctrine#8884
greg0ire pushed a commit to scyzoryck/orm that referenced this issue Aug 21, 2021
…ine/dbal:3.x.

Generating `getGuidExpression` has been removed in doctrine/dbal:3.x.

Partially fixes doctrine#8884
greg0ire pushed a commit that referenced this issue Aug 22, 2021
…ine/dbal:3.x. (#8898)

Generating `getGuidExpression` has been removed in doctrine/dbal:3.x.

Partially fixes #8884
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants