Skip to content

[Notifier] Fix component version constraint in bridges #39571

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.

Sign up for GitHub

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
Merged
merged 1 commit into from
Dec 22, 2020
Merged

[Notifier] Fix component version constraint in bridges #39571

merged 1 commit into from
Dec 22, 2020

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Dec 18, 2020 •
edited
Loading

Q A
Branch? 5.x
Bug fix? no
New feature? no
Deprecations? np
Tickets -
License MIT
Doc PR -

Composer does not resolve ~5.3.0 to 5.x-dev with --prefer-lowest actually.

@chalasr chalasr added this to the 5.x milestone Dec 18, 2020
@derrabus
Copy link
Member

😭

Would setting a branch alias on the notifier package help?

@chalasr
Copy link
Member Author

chalasr commented Dec 19, 2020

@derrabus Probably, but I'll let @nicolas-grekas confirm it's desirable.
Anyway, do we really want to forbid 5.4.x and later?

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 19, 2020 •
edited
Loading

How does composer behave if we use the branch-version entry to say this is 5.3.x-dev?

@chalasr chalasr force-pushed the notifier-req branch 5 times, most recently from f7a5a2f to b907f97 Compare December 19, 2020 11:27
@chalasr
Copy link
Member Author

chalasr commented Dec 19, 2020

@nicolas-grekas Tried using "branch-version": "5.3.x", first on the root's composer.json then on the Notifier's root one. Either way, it didn't help (any chance we need to merge that entry first to see the result actually?)

@OskarStark OskarStark mentioned this pull request Dec 21, 2020
Merged
nicolas-grekas
nicolas-grekas reviewed Dec 22, 2020
View reviewed changes
src/Symfony/Component/Notifier/Bridge/Discord/composer.json Outdated
@@ -18,7 +18,7 @@
"require": {
"php": ">=7.2.5",
"symfony/http-client": "^4.3|^5.0",
"symfony/notifier": "~5.3.0"
"symfony/notifier": "^5.3"
Copy link
Member

Choose a reason for hiding this comment

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

Sure, this fixes it, but this defeats the preemptive protection against BC breaks in 5.4...

Copy link
Member

Choose a reason for hiding this comment

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

^5.3,<5.4 ?

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 doesn't work (same with a conflict rule) or any combination that forbids >=5.4.
5.x is literally interpreted as 5+ by composer, which may imply 5.4.

Copy link
Member

Choose a reason for hiding this comment

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

hmm.. sounds logic.
We can't both accept 5.x today because it points to 5.3 and refuse 5.x when it will point to 5.4.

Is there something to do with branch aliases?

derrabus
derrabus approved these changes Dec 22, 2020
View reviewed changes
Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

I'd like to merge this PR as it is now because I'd like to unblock our CI.

If before 5.3 RC we decide that the component remains experimental, we can branch out 5.3 and revert this PR.

@chalasr chalasr merged commit af43335 into symfony:5.x Dec 22, 2020
@chalasr chalasr deleted the notifier-req branch December 22, 2020 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@jderusse jderusse jderusse left review comments

@derrabus derrabus derrabus approved these changes

Assignees
No one assigned
Labels
Notifier Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
5 participants
@chalasr @derrabus @nicolas-grekas @jderusse @carsonbot