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

Jump to bottom

[Maintenance] Add support for doctrine/dbal:^3.0 #14145

Merged
merged 10 commits into from Jul 20, 2022
Merged

[Maintenance] Add support for doctrine/dbal:^3.0 #14145

merged 10 commits into from Jul 20, 2022

Conversation

jakubtobiasz
Copy link
Member

@jakubtobiasz jakubtobiasz commented Jul 11, 2022 •
edited

Q A
Branch? 1.12
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Related tickets
License MIT

This PR adds support for doctrine/dbal:^3.0. Some tests are failing and I have to fix them but it's ready to review the approach.

Removing NULL COMMENT \'(DC2Type:json_array)\' was necessary because setting up a fresh database using migrations was failing.

@jakubtobiasz jakubtobiasz requested a review from a team as a code owner July 11, 2022 16:37
@jakubtobiasz
Copy link
Member Author

jakubtobiasz commented Jul 12, 2022 •
edited

It seems support for dbal3 works (and dbal2, too!). Two areas required changes:

  1. Migrations (as they stopped working after upgrade to dbal3)
  2. Deprecations

Migrations
I had to remove every occurrence of COMMENT \'(DC2Type:json_array)\' as it was trying to "cast" column type to json_array (removed in dbal3). Due to it, we couldn't perform even a fresh install of Sylius.

Deprecations
Psalm was complaining about deprecations, but I decided to fix them as it might be easier to upgrade to a newer version in the future. I decided to create AbstractMySqlMigration class and add a logic behind checking if current platform is MySql and/or MariaDB. Also I had to add some psalm-suppress and phpstan-ignore annotations because in dbal2 we had MySqlPlatform and now in dbal3 we have MySQLPlatform.

@jakubtobiasz jakubtobiasz changed the title [WIP][Maintenance] Add support for doctrine/dbal:^3.0 [Maintenance] Add support for doctrine/dbal:^3.0 Jul 12, 2022
@probot-autolabeler probot-autolabeler bot added the Maintenance CI configurations, READMEs, releases, etc. label Jul 12, 2022
GSadee
GSadee approved these changes Jul 14, 2022
View reviewed changes
app/migrations/Version20170116215417.php Outdated Show resolved Hide resolved
stloyd
stloyd suggested changes Jul 20, 2022
View reviewed changes
src/Sylius/Bundle/CoreBundle/Migrations/Version20220407131547.php Outdated
@@ -57,7 +53,7 @@ public function down(Schema $schema): void

private function getExistingIndexesNames(string $tableName): array
{
$indexes = $this->connection->getSchemaManager()->listTableIndexes($tableName);
$indexes = $this->connection->createSchemaManager()->listTableIndexes($tableName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing fallback.

Copy link
Member

Choose a reason for hiding this comment

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

Should be probably something like this 🖖

@GSadee GSadee merged commit 204c60b into Sylius:1.12 Jul 20, 2022
@GSadee
Copy link
Member

GSadee commented Jul 20, 2022

Thanks, Jakub! 🥇

@Rafikooo Rafikooo mentioned this pull request Apr 20, 2023
Merged
jakubtobiasz added a commit that referenced this pull request Apr 21, 2023
@jakubtobiasz
minor #14962 [ApiBundle][Maintenance] Remove conflict with `doctrine/…
dd111b5 
…dbal:3.*` (Rafikooo)

This PR was merged into the 1.12 branch.

Discussion
----------

| Q               | A                                                            |
|-----------------|--------------------------------------------------------------|
| Branch?         | 1.12 and 1.13 <!-- see the comment below -->                  |
| Bug fix?        | no                                                      |
| New feature?    | no                                                      |
| BC breaks?      | no                                                      |
| Deprecations?   | no<!-- don't forget to update the UPGRADE-*.md file --> |
| License         | MIT                                                          |

It should probably be removed along with #14145

Commits
-------
  [ApiBundle][Maintenance] Remove conflict with doctrine/dbal:3.*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zales0123 Zales0123 Zales0123 left review comments

@lchrusciel lchrusciel lchrusciel left review comments

@stloyd stloyd stloyd requested changes

@GSadee GSadee GSadee approved these changes

Assignees
No one assigned
Labels
Maintenance CI configurations, READMEs, releases, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jakubtobiasz @GSadee @stloyd @Zales0123 @lchrusciel