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

[Cache][Doctrine][DoctrineBridge][Lock][Messenger] Compatibility with ORM 3 and DBAL 4 #51947

Merged
merged 1 commit into from Oct 12, 2023

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented Oct 10, 2023

Q A
Branch? 5.4
Bug fix? no
New feature? no
Deprecations? no
Tickets N/A
License MIT

This PR makes the 5.4 branch compatible with ORM 3 and DBAL 4.

Note: On the 5.4 branch, I cannot add ORM 3 to the root composer.json because ORM 3 requires VarExporter 6. So, only high-deps tests it is. 🤷🏻‍♂️

@carsonbot carsonbot added this to the 5.4 milestone Oct 10, 2023
@derrabus derrabus changed the title [DoctrineBridge] Run high-deps tests with ORM 3 [DoctrineBridge] Run high-deps tests with ORM 3 and DBAL 4 Oct 10, 2023
@derrabus derrabus added the Cache label Oct 10, 2023
@carsonbot carsonbot changed the title [DoctrineBridge] Run high-deps tests with ORM 3 and DBAL 4 [Cache][DoctrineBridge] Run high-deps tests with ORM 3 and DBAL 4 Oct 10, 2023
@derrabus derrabus added the Lock label Oct 10, 2023
@carsonbot carsonbot changed the title [Cache][DoctrineBridge] Run high-deps tests with ORM 3 and DBAL 4 [Cache][DoctrineBridge][Lock] Run high-deps tests with ORM 3 and DBAL 4 Oct 10, 2023
@carsonbot carsonbot changed the title [Cache][DoctrineBridge][Lock] Run high-deps tests with ORM 3 and DBAL 4 [Cache][DoctrineBridge][Lock][Messenger] Run high-deps tests with ORM 3 and DBAL 4 Oct 10, 2023
@derrabus derrabus force-pushed the bump/orm-3 branch 2 times, most recently from b543f97 to ec0f818 Compare October 10, 2023 12:18
'SELECT a.* FROM (SELECT m.id AS "id", m.body AS "body", m.headers AS "headers", m.queue_name AS "queue_name", m.created_at AS "created_at", m.available_at AS "available_at", m.delivered_at AS "delivered_at" FROM messenger_messages m WHERE (m.delivered_at is null OR m.delivered_at < ?) AND (m.available_at <= ?) AND (m.queue_name = ?)) a WHERE ROWNUM <= 50',
];
if (!class_exists(MySQL57Platform::class)) {
// DBAL >= 4
Copy link
Member

Choose a reason for hiding this comment

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

Can't we detect the Oracle changes based on something related to OraclePlatform rather than checking for the removal of MySQL57Platform ?

Copy link
Member Author

Choose a reason for hiding this comment

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

If you have an idea, please let me know. 🙂

@@ -391,7 +393,7 @@ public function testGeneratedSql(AbstractPlatform $platform, string $expectedSql
public static function providePlatformSql(): iterable
{
yield 'MySQL' => [
Copy link
Member

Choose a reason for hiding this comment

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

DBAL 4+ separates MariaDBPlatform from MySQLPlatform. So we should add a test for it in the data provider to avoid loosing support for MariaDB.

Copy link
Member Author

Choose a reason for hiding this comment

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

That test is already there, scroll down a little.

@stof
Copy link
Member

stof commented Oct 10, 2023

I'm wondering whether it make sense to do that work in the 5.4 branch while 6.4 is likely to be the active LTS by the time of the Doctrine major releases.

@derrabus derrabus force-pushed the bump/orm-3 branch 2 times, most recently from 11c0e64 to f5266ab Compare October 10, 2023 13:20
@derrabus
Copy link
Member Author

derrabus commented Oct 10, 2023

I'm wondering whether it make sense to do that work in the 5.4 branch while 6.4 is likely to be the active LTS by the time of the Doctrine major releases.

We will maintain ORM 2 for quite a while, so only making Symfony 6 compatible with ORM 3 would be fine. A big problem is though, that no Symfony 5 package (except VarExporter) is actively conflicting with ORM 3. It's a dev dependency everywhere. So basically, people will be able to install ORM 3 with Symfony 5 and flood our tracker with bug reports. 😢

@carsonbot carsonbot changed the title [Cache][DoctrineBridge][Lock][Messenger] Run high-deps tests with ORM 3 and DBAL 4 [Cache][Doctrine][DoctrineBridge][Lock][Messenger] Run high-deps tests with ORM 3 and DBAL 4 Oct 11, 2023
@OskarStark
Copy link
Contributor

We could explicitly conflict with ORM 3 to avoid such situation

@derrabus derrabus changed the title [Cache][Doctrine][DoctrineBridge][Lock][Messenger] Run high-deps tests with ORM 3 and DBAL 4 [Cache][Doctrine][DoctrineBridge][Lock][Messenger] Compatibility with ORM 3 and DBAL 4 Oct 12, 2023
@derrabus
Copy link
Member Author

derrabus commented Oct 12, 2023

We could explicitly conflict with ORM 3 to avoid such situation

We could've done that two years ago, yes. If we do that now, Composer will just pin an old Doctrine Bridge and move on.

@nicolas-grekas
Copy link
Member

Thank you @derrabus.

@nicolas-grekas nicolas-grekas merged commit 6dfc142 into symfony:5.4 Oct 12, 2023
8 of 11 checks passed
@derrabus derrabus deleted the bump/orm-3 branch October 12, 2023 16:08
nicolas-grekas added a commit that referenced this pull request Oct 13, 2023
This PR was merged into the 5.4 branch.

Discussion
----------

[DoctrineBridge] Fix DBAL 4 compatibility

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | N/A
| License       | MIT

This PR backports DBAL 4 compatibility changes from #51997 that were not in #51947. Sorry for messing this up.

Commits
-------

50c0fbc Fix DBAL 4 compatibility
@fabpot fabpot mentioned this pull request Oct 21, 2023
@fabpot fabpot mentioned this pull request Oct 29, 2023
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.

None yet

5 participants