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

Add conflict with doctrine/dbal ^3 to avoid missing json_array doctrine type error #13215

Merged
merged 2 commits into from Oct 27, 2021

Conversation

Prometee
Copy link
Contributor

@Prometee Prometee commented Oct 18, 2021

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.

@4c0n
Copy link
Contributor

4c0n commented Oct 21, 2021

@Prometee Wouldn't it be better to add a conflict to the conflict section and document it in CONFLICTS.md?
If not, why version 2.6 and not the currently maintained version 2.13?

https://github.com/Sylius/Sylius/blob/1.9/CONFLICTS.md

@Prometee
Copy link
Contributor Author

@4c0n yes you are right it will be better to know when and why to remove it one day.
About the version, I take advise from this comment : #13211 (comment)
Can you test the package version you suggested, I'l update the PR if it's the case.

@4c0n
Copy link
Contributor

4c0n commented Oct 21, 2021

@Prometee If you use ^2.6 I think it would resolve to the latest minor version of major version 2 anyway, so even if you use ^2.6 you are already getting 2.13.4 (https://packagist.org/packages/doctrine/dbal#2.13.4) unless there is a lock file stating otherwise. Was just wondering if there was a specific reason to specify that version. So I guess there's not much of a point testing it, especially if you are going to change it to a conflict.

Hope this can get resolved soon, it's causing a lot of extra work for us all, thanks for taking charge. 😃

@probot-autolabeler probot-autolabeler bot added the Maintenance CI configurations, READMEs, releases, etc. label Oct 21, 2021
@Prometee Prometee changed the title Constraint doctrine/dbal to v2 to avoid missing json_array type error Add conflict with doctrine/dbal ^3 to avoid missing json_array doctrine type error Oct 21, 2021
@vvasiloi
Copy link
Contributor

@4c0n @Prometee I think that the minimum version should be the same as the one from doctrine/orm, because that's where Sylius gets doctrine/dbal. Sylius 1.9 requires doctrine/orm:^2.7, which in turn requires doctrine/dbal:^2.9.3, so that should be the minimum requirement in Sylius too. WDYT?

@4c0n
Copy link
Contributor

4c0n commented Oct 21, 2021

@vvasiloi So ^2.7 can/will resolve to https://github.com/doctrine/orm/blob/f346379c7bf39a9f46771cfd9043e0eb2dd98793/composer.json#L26 (https://github.com/doctrine/orm/blob/2.10.1/composer.json)
especially without lock file. Which in turn has the problem of also allowing the dbal version with the issue at hand.

So specifying the conflict on 3.x versions solves the problem and still allows for all the other versions of dbal to be installed.
Unless this package directly uses dbal somehow/somewhere it should not be listed as a direct dependency. If that was the case I think it would make sense what you're suggesting 😃

@vvasiloi
Copy link
Contributor

@4c0n There shouldn't be an issue if Sylius requires doctrine/dbal ^2.9.3, which is the requirement for doctrine/orm 2.7.0, which in turn is the current minimum requirement in Sylius. A conflict with doctrine/dbal >= 3 is much better though.

@vvasiloi
Copy link
Contributor

Either way, it's better to eventually add an explicit requirement to doctrine/dbal in Sylius.

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 would say, that requiring dbal is a better approach then to conflict it, but let's unblock all of us with this PR

@lchrusciel lchrusciel merged commit 6923e75 into Sylius:1.9 Oct 27, 2021
@lchrusciel
Copy link
Member

Thanks, Francis! 🥇

@maximehuran
Copy link
Contributor

When this PR will be released ? This issue is blocking our plugin github actions :(

Zales0123 added a commit that referenced this pull request Oct 28, 2021
…ement (lchrusciel)

This PR was merged into the 1.9 branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | 1.9
| Bug fix?        | yes
| New feature?    | no
| BC breaks?      | no
| Deprecations?   | no
| Related tickets | mentioned in #13215 (review)
| License         | MIT

<!--
 - Bug fixes must be submitted against the 1.9 or 1.10 branch (the lowest possible)
 - Features and deprecations must be submitted against the master branch
 - Make sure that the correct base branch is set

 To be sure you are not breaking any Backward Compatibilities, check the documentation:
 https://docs.sylius.com/en/latest/book/organization/backward-compatibility-promise.html
-->


Commits
-------

cb7cac8 [Maintenance] Replace dbal conflict with explicit requirement
@oallain
Copy link
Member

oallain commented Oct 28, 2021

When this PR will be released ? This issue is blocking our plugin github actions :(

Same for me :(

lruozzi9 added a commit to webgriffe/SyliusClerkPlugin that referenced this pull request Oct 28, 2021
@maximehuran
Copy link
Contributor

maximehuran commented Oct 28, 2021

@oallain I found a way to trick it

  • Put conflict in the plugin's composer.json
  • Create sylius project without install
  • Require my plugin without install (neither update)
  • Run composer install

This will install DBAL 2.x while resolving dependencies

monsieurbiz/SyliusRichEditorPlugin#168

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance CI configurations, READMEs, releases, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants