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

Replace json_array with json type as requested by the deprecation #13214

Merged
merged 5 commits into from
Jan 10, 2022

Conversation

Prometee
Copy link
Contributor

Q A
Branch? 1.9, 1.10
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Related tickets fixes #13211
License MIT

doctrine/dbal:"^3" remove json_array field type, previously it was deprecated in favor of json field type.

@Prometee Prometee requested a review from a team as a code owner October 18, 2021 13:40
@Prometee Prometee force-pushed the column-type-json-array-to-json branch from f857c2e to c8f2bee Compare October 18, 2021 14:07
@Prometee
Copy link
Contributor Author

Their is still an issue with sylius_gateway_config table containing a remaining json_array type, we will have to wait for Payum/Payum#920 to be merged and released to fix this issue.

@vvasiloi
Copy link
Contributor

Maybe add a constraint for doctrine/dbal for now?
I was surprised that "doctrine/orm": "^2.7" installed doctrine/dbal v3.

@Prometee
Copy link
Contributor Author

@vvasiloi you are right it's the temporary fix, maybe another PR is required because I also think this PR will be needed in the futur when Payum/Payum#920 will be available.

@vvasiloi
Copy link
Contributor

@Prometee yes, better do it in a separate PR with either an explicit require constraint, or a conflict.
This will also prevent the plugins without a composer.lock file (which are the most if not all) from breaking.

@Prometee
Copy link
Contributor Author

@vvasiloi done !

#13215

@mbabker
Copy link
Contributor

mbabker commented Oct 18, 2021

I was surprised that "doctrine/orm": "^2.7" installed doctrine/dbal v3.

Why? One of the main features of ORM 2.10 (which is already blocked on the 1.10 branch of this repo because of the gedmo/doctrine-extensions package being incompatible) is DBAL 3.x compatibility and DBAL 3.x should only be installed if all dependencies allow it. ORM 2.9 and earlier don't allow DBAL 3.x.

In general, Sylius' dependency declarations are really too loose IMO (in this repo, the standalone package repos, and the skeleton repos which these only declare sylius/sylius as their dependency (meaning out-of-the-box they rely on this repo to get the dependencies right) as a lot of direct dependencies seem to be not declared at all and things are relying heavily on transient dependencies to get it right (two practical examples include the dependencies on babdev/pagerfanta-bundle and jms/serializer-bundle but no declarations on pagerfanta/* or jms/serializer, which is where most of the code being used actually comes from).

@vvasiloi
Copy link
Contributor

Maybe we should introduce Sylius to https://github.com/maglnet/ComposerRequireChecker?

@Prometee Prometee changed the title Replace json_array type with json type as requested by the deprecation Replace json_array with json type as requested by the deprecation Oct 21, 2021
@mmenozzi
Copy link
Contributor

Maybe we should introduce Sylius to https://github.com/maglnet/ComposerRequireChecker?

@vvasiloi @mbabker Yes to avoid this kind of issues ComposerRequireChecker would be useful. Indeed I tried to introduce it in Sylius with #10664 2 years ago but the PR is still there, open and not reviewed.

@mmenozzi
Copy link
Contributor

@vvasiloi @mbabker @Prometee note that even when this PR will be merged there will be also this issue with doctrine/dbal >= 3 and PHP 8: doctrine/migrations#1202

@vvasiloi
Copy link
Contributor

@mmenozzi there's this PR #13215 that will add a conflict with doctrine/dbal >= 3 until all the issues are solved.

@mmenozzi
Copy link
Contributor

@mmenozzi there's this PR #13215 that will add a conflict with doctrine/dbal >= 3 until all the issues are solved.

Yes I noticed it. Do you think that we should open an issue here to keep track of that?

lchrusciel added a commit that referenced this pull request Oct 27, 2021
…array` doctrine type error (Prometee)

This PR was merged into the 1.9 branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | 1.9, 1.10
| Bug fix?        | yes
| New feature?    | no
| BC breaks?      | no
| Deprecations?   | no
| Related tickets | fixes #13211
| License         | MIT

See #13211 and #13214 for more details about the `json_array` type error.

Commits
-------

4466a76 Add conflict with doctrine/dba ^3 to avoid missing json_array type error
b7b7095 Merge branch '1.9' into constraint-doctrine-dbal
@lchrusciel
Copy link
Member

Hey Francis,

can you fix the build, remove dbal conflict and add one additional workflow to check if the new configuration works properly with dbal 2.10/2.9 and 3.0?

@probot-autolabeler probot-autolabeler bot added Documentation Documentation related issues and PRs - requests, fixes, proposals. Maintenance CI configurations, READMEs, releases, etc. labels Nov 12, 2021
@Prometee Prometee force-pushed the column-type-json-array-to-json branch from 8f1504b to 3c1c623 Compare November 12, 2021 17:19
@Prometee Prometee closed this Nov 12, 2021
@Prometee Prometee force-pushed the column-type-json-array-to-json branch from 3c1c623 to f560e16 Compare November 12, 2021 17:23
@Prometee Prometee reopened this Nov 12, 2021
@Prometee
Copy link
Contributor Author

Prometee commented Nov 12, 2021

Hi @lchrusciel,

I remove 1 commit about conflict and rebase the branch with the 1.9 one , but the build seems to fail because of another issue. (Nevermind it's failling because of payum/payum : https://github.com/Payum/Payum/blob/master/src/Payum/Core/Bridge/Doctrine/Resources/mapping/ArrayObject.orm.xml#L5)
When you said "add one additional workflow", you mean GitHub Action workflow ? I can't see any existing workflow doing what you asked for. Can you enlighten me ?

@CoderMaggie CoderMaggie removed the Documentation Documentation related issues and PRs - requests, fixes, proposals. label Nov 15, 2021
@Prometee
Copy link
Contributor Author

Prometee commented Dec 2, 2021

Waiting for this PR : Payum/Payum#924 to be merged and released.

@maximehuran
Copy link
Contributor

Hi,

On a fresh Sylus installation with MySQL 8, I have a database schema is not in sync with the current mapping file.

Here is the generated migration with a diff :

<?php

declare(strict_types=1);

namespace App\Migrations;

use Doctrine\DBAL\Schema\Schema;
use Doctrine\Migrations\AbstractMigration;

/**
 * Auto-generated Migration: Please modify to your needs!
 */
final class Version20220107153032 extends AbstractMigration
{
    public function getDescription(): string
    {
        return '';
    }

    public function up(Schema $schema): void
    {
        // this up() migration is auto-generated, please modify it to your needs
        $this->addSql('ALTER TABLE sylius_gateway_config CHANGE config config JSON NOT NULL');
    }

    public function down(Schema $schema): void
    {
        // this down() migration is auto-generated, please modify it to your needs
        $this->addSql('ALTER TABLE sylius_gateway_config CHANGE config config JSON CHARACTER SET utf8 NOT NULL COLLATE `utf8_unicode_ci` COMMENT \'(DC2Type:json_array)\'');
    }
}

FYI I have added this on my composer.json :

    "conflict": {
        "doctrine/dbal": "^3"
    }

@Prometee Prometee force-pushed the column-type-json-array-to-json branch from 9fdcc08 to 661b8e7 Compare January 10, 2022 14:52
@probot-autolabeler probot-autolabeler bot added the Documentation Documentation related issues and PRs - requests, fixes, proposals. label Jan 10, 2022
@Prometee Prometee changed the base branch from 1.9 to 1.10 January 10, 2022 14:52
@vvasiloi vvasiloi removed the Documentation Documentation related issues and PRs - requests, fixes, proposals. label Jan 10, 2022
@vvasiloi vvasiloi added Bug Confirmed bugs or bugfixes. Critical Issues and PRs, which are critical and should be fixed ASAP. Dependencies Pull requests that update a dependency file and removed Maintenance CI configurations, READMEs, releases, etc. labels Jan 10, 2022
@vvasiloi
Copy link
Contributor

@lchrusciel the failed jobs don't seem to be related to the changes. Maybe we should restart the workflow.

Copy link
Member

@lchrusciel lchrusciel left a comment

Choose a reason for hiding this comment

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

I've just restarted them.

Probably we should also allow dbal v3 with these changes. But that may be done in separate PR. Still, we need to have green build and composer is making some problems from what I see

@probot-autolabeler probot-autolabeler bot added the Maintenance CI configurations, READMEs, releases, etc. label Jan 10, 2022
@Prometee
Copy link
Contributor Author

@lchrusciel yes I already retest it on my local env and all BeHat JS are green. I guess their is some weird network issue happening here. Restarting the build can help if it fails again...

@vvasiloi
Copy link
Contributor

@lchrusciel all green.

@lchrusciel lchrusciel merged commit 5e79b5f into Sylius:1.10 Jan 10, 2022
@lchrusciel
Copy link
Member

Thank you, Francis! 🎉

@lchrusciel
Copy link
Member

TBH, I should just mention all of you. So thank you all Francis, Joseph, Victor, Manuele, Michael and Maxime for your work, comments, spotting problems etc :)

@Prometee Prometee deleted the column-type-json-array-to-json branch January 10, 2022 19:03
@vvasiloi
Copy link
Contributor

@lchrusciel this should go to 1.11 and master too.

@lchrusciel
Copy link
Member

I have some issues with upmerging atm. I've contacted @Zales0123 & @GSadee to do it, they should do it tomorrow morning. Also, we should release a new version of Sylius

GSadee added a commit to Sylius/Sylius-Standard that referenced this pull request Jan 14, 2022
…sciel)

This PR was merged into the 1.10-dev branch.

Discussion
----------

Content of PR:
- Changes to PHP, Node and Sf versions
- Usage of sync messenger transport in testing required for proper testing results
- Temporary removal of schema validation waiting for the Sylius release after: Sylius/Sylius#13214
- Whitespace removal in Behat tags - could be omitted, as it is minor stuff, but deprecation error was driving me nuts
- Due to some strange issue with test test execution(ref. https://github.com/Sylius/Sylius-Standard/actions/runs/1688323707) the [vendor/sylius/sylius/features/promotion/managing_catalog_promotions/toggling_catalog_promotion.feature:35](https://github.com/Sylius/Sylius/blob/54dcc4f2ed0a4fea5908fbea3bd69a0f27f16fa1/features/promotion/managing_catalog_promotions/toggling_catalog_promotion.feature#L35-L40) scenario broke in the UI(was fine in API), while was green when I trigger it locally. While run in separation, it was green also on CI, so I've decided to split it to separate run in CI what solved the issue 
- I had to add webprofiler to test cached env due to changes introduced here: Sylius/Sylius#12990. It is needed to register TracableMessegeBus for testing purposes

The build is finally green, but TBH I'm mindblown with the strangeness of errors. If anyone would like to solve them better, be my guests.
  
For the two last issues I've alse left comments in commits for future understanding of problems

Commits
-------

c20eed1 [Maintenance] Run GitHub actions on PHP 8, Node 14 & Symfony 5.4
dc2d180 [Config] Use doctrine transport as default for Symfony Messenger
92d9b2d [Config] Use sync transport for test env
32c5c8c [Maintenance] Temporary removal of schema validation due to changes in payum to support dbal v3
69b3b45 [Maintenance] Remove whitespace in tag defintion
468e37a [Behat] Run managing catalog promotion scenarios in separation
b910859 [Behat] Import web profiler test config into test cached
delyriand pushed a commit to maximehuran/SyliusColishipPlugin that referenced this pull request Apr 8, 2022
windragon0910 added a commit to windragon0910/symfony_ecom_framework that referenced this pull request Oct 25, 2023
…sciel)

This PR was merged into the 1.10-dev branch.

Discussion
----------

Content of PR:
- Changes to PHP, Node and Sf versions
- Usage of sync messenger transport in testing required for proper testing results
- Temporary removal of schema validation waiting for the Sylius release after: Sylius/Sylius#13214
- Whitespace removal in Behat tags - could be omitted, as it is minor stuff, but deprecation error was driving me nuts
- Due to some strange issue with test test execution(ref. https://github.com/Sylius/Sylius-Standard/actions/runs/1688323707) the [vendor/sylius/sylius/features/promotion/managing_catalog_promotions/toggling_catalog_promotion.feature:35](https://github.com/Sylius/Sylius/blob/54dcc4f2ed0a4fea5908fbea3bd69a0f27f16fa1/features/promotion/managing_catalog_promotions/toggling_catalog_promotion.feature#L35-L40) scenario broke in the UI(was fine in API), while was green when I trigger it locally. While run in separation, it was green also on CI, so I've decided to split it to separate run in CI what solved the issue 
- I had to add webprofiler to test cached env due to changes introduced here: Sylius/Sylius#12990. It is needed to register TracableMessegeBus for testing purposes

The build is finally green, but TBH I'm mindblown with the strangeness of errors. If anyone would like to solve them better, be my guests.
  
For the two last issues I've alse left comments in commits for future understanding of problems

Commits
-------

c20eed169b7da9ce6307811e79d5ff28d535bb8e [Maintenance] Run GitHub actions on PHP 8, Node 14 & Symfony 5.4
dc2d180d062bae06811b82766a9a710f62949bf4 [Config] Use doctrine transport as default for Symfony Messenger
92d9b2daa305008d8e378b713dbe95108ac7ad9c [Config] Use sync transport for test env
32c5c8c82d7eb3a415910ab2bf75747b46283a4d [Maintenance] Temporary removal of schema validation due to changes in payum to support dbal v3
69b3b45f03dbe6ad303e019509d6d63e75193c76 [Maintenance] Remove whitespace in tag defintion
468e37ac7d2a6813457fe5df9579268d445c31cd [Behat] Run managing catalog promotion scenarios in separation
b910859efede1abe80b613c2fbda835118e32a0b [Behat] Import web profiler test config into test cached
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Confirmed bugs or bugfixes. Critical Issues and PRs, which are critical and should be fixed ASAP. Dependencies Pull requests that update a dependency file Maintenance CI configurations, READMEs, releases, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants