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

Run builds with DBAL 4.0.x-dev #9727

Merged
merged 5 commits into from
May 6, 2022
Merged

Run builds with DBAL 4.0.x-dev #9727

merged 5 commits into from
May 6, 2022

Conversation

morozov
Copy link
Member

@morozov morozov commented May 3, 2022

Relevant changes from DBAL 4.0.x:

  1. Connection::quote() can only quote strings dbal#3488
  2. Add proper types to Doctrine\DBAL\Types namespace. dbal#3569
  3. Getting rid of the column name index dbal#3583

TODO:

Improve DBAL upgrade path:

Next steps:

  1. Build with DBAL 4@dev on MySQL and MariaDB #9735.
  2. Test the build with ^4@dev on PostgreSQL.
  3. Run static analysis with ^4@dev.
  4. Bump DBAL version constraint to ^3.4 once it's released.

composer.json Outdated Show resolved Hide resolved
@morozov
Copy link
Member Author

morozov commented May 3, 2022

@derrabus do you have a recommendation on addressing this static analysis issue?

lib/Doctrine/ORM/Persisters/Entity/SingleTablePersister.php:144:44: RedundantCastGivenDocblockType: Redundant cast to string given docblock-provided type

It's related to this bullet point:

Why do discriminator maps have numeric keys despite their <string,class-string> type?

One of the non-string occurrences is addressed by adding this:

unset($classAnnotations[$key]);

But there are more.

@derrabus
Copy link
Member

derrabus commented May 4, 2022

@derrabus do you have a recommendation on addressing this static analysis issue?

This is a known issue with discriminator maps. Baseline it for now, fixing it is out of scope for this PR.

composer.json Outdated
@@ -25,7 +25,7 @@
"ext-ctype": "*",
"doctrine/collections": "^1.5",
"doctrine/common": "^3.3",
"doctrine/dbal": "^3.3",
"doctrine/dbal": "^3.4.x-dev",
Copy link
Member Author

@morozov morozov May 4, 2022

Choose a reason for hiding this comment

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

Would it be acceptable to bump this version constraint temporarily (we can pin it to a specific commit)?

I plan to add a couple more upgrade path improvements to the DBAL 3.4 (see the description). Since they are not yet released, if we don't bump the constraint, we will either have to force the release in order to avoid the dependency of the dev ORM on dev DBAL, or we will have to refrain from merging all the changes related to the DBAL 4 until they are implemented in full and the scope of the upgrade path improvements is fully defined.

I believe this change is acceptable since it's unlikely that ORM 3 will be released before DBAL 3.4. Even if this happens, we can release DBAL 3.4 right before that.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine, as you say we can still force publish 3.4 in the unlikely event we want to publish ORM 3 before it is ready.

Copy link
Member

Choose a reason for hiding this comment

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

This is fine, but let's use this constraint instead:

Suggested change
"doctrine/dbal": "^3.4.x-dev",
"doctrine/dbal": "^3.4@dev",

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we can use that. How is it better than the original constraint?

Copy link
Member

Choose a reason for hiding this comment

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

It's consitent with what we usually use in those situations.

@morozov morozov marked this pull request as ready for review May 4, 2022 16:21
@morozov morozov requested review from derrabus and greg0ire May 5, 2022 13:56
composer.json Outdated
@@ -25,7 +25,7 @@
"ext-ctype": "*",
"doctrine/collections": "^1.5",
"doctrine/common": "^3.3",
"doctrine/dbal": "^3.3",
"doctrine/dbal": "^3.4.x-dev",
Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine, as you say we can still force publish 3.4 in the unlikely event we want to publish ORM 3 before it is ready.

@@ -1063,6 +1063,9 @@
<PossiblyUndefinedVariable occurrences="1">
<code>$columnList</code>
</PossiblyUndefinedVariable>
<RedundantCastGivenDocblockType occurrences="1">
<code>(string) $discrValues[$subclassName]</code>
</RedundantCastGivenDocblockType>
Copy link
Member

Choose a reason for hiding this comment

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

This should be fixed up into b291c4e

@derrabus
Copy link
Member

derrabus commented May 5, 2022

We want the 2.x series to run deprecation-free with DBAL 3.x. This PR seems to prepare the ORM for some deprecations coming with DBAL 3.4.0. Can we backport parts of this PR to 2.13.x?

@morozov
Copy link
Member Author

morozov commented May 5, 2022

Yeah, about half of these commits could be backported to 2.13. Even though some of them wouldn't address runtime deprecations, they would set an example of writing code compatible with DBAL 4 (e.g. 9f0ff98, 96b1c37, 99aa247).

I can start with backporting and once they are merged back up, we can continue here. In the meantime, I can address any issues regarding the code changes.

@derrabus
Copy link
Member

derrabus commented May 5, 2022

Sounds good.

@derrabus derrabus merged commit 5d860bf into doctrine:3.0.x May 6, 2022
@morozov morozov deleted the dbal-4.0.x-dev branch May 6, 2022 13:25
@derrabus derrabus added this to the 3.0.0 milestone May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants