-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Security] don't allow to install the split Security packages #16144
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
Conversation
Another solution would be to have Btw, this is needed at the root too, not only in the component |
"conflict": { | ||
"symfony/security-acl": "*", | ||
"symfony/security-core": "*", | ||
"symfony/security-csrf": "*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one is wrong. security-csrf
was new code, which does not exist in the 2.3 branch
@stof Which solution is more effective for Composer (i.e. what requires less work for the dependency resolver)? Using |
I removed the CSRF component and duplicated the config into the root I also talked to @alcohol who pointed my to composer/composer#3754. Isn't that a reason we should use conflict rules here? |
@xabbuh the advantage of If you want to workaround the bug of composer, you would have to use a conflict rule with |
Well, my goal of this pull request was to forbid to use a |
@xabbuh even without the bug, Composer would not forbid it: |
@stof I seem to be missing something. When we add the conflict rule for On the other hand, when we tell the full package to replace |
@xabbuh assuming that the bug does not exist, it should forbid installing any version of the replaced package when the replacer is installed. |
Sure, that's why I suggest to make this change (once the conflict rule is added, the above described scenario would not install anymore). Or we could add a replace rule like you suggest which requires a fix in Composer to have an effect. |
Is this one mergeable as is? |
I think yes, but we can change the |
@xabbuh I think |
I would use a replace for self.version and a conflict for !self.version, I think. |
@alcohol the conflict for |
Ideally, it should not, no. But right now it does. Hence why I suggest it. It's harmless to add it yourself, and I doubt the odd behaviour in Composer will get fixed any time soon. |
You can't easily conflict with !self.version though AFAIK, so I would rather people not attempt to fix composer stuff from the outside. replace is used for everything else in symfony subtree splits, so I don't see why not continuing in that line for this? |
Currently, you would be able to install the Security component fromm Symfony 2.3 together with one of the split packages from a higher Symfony vesion like this: ```json { "require": { "symfony/symfony": "2.3.*", "symfony/security-core": "~2.7" } } ``` However, you will end up with classes being present twice. This must be reverted after merging up in the `2.7` branch.
I updated the PR to use |
Thank you @xabbuh. |
…ges (xabbuh) This PR was merged into the 2.3 branch. Discussion ---------- [Security] don't allow to install the split Security packages | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #16134 | License | MIT | Doc PR | Currently, you would be able to install the Security component fromm Symfony 2.3 together with one of the split packages from a higher Symfony vesion like this: ```json { "require": { "symfony/symfony": "2.3.*", "symfony/security-core": "~2.7" } } ``` However, you will end up with classes being present twice. This must be reverted after merging up in the `2.7` branch. Commits ------- 0d14064 don't allow to install the split Security packages
Currently, you would be able to install the Security component fromm
Symfony 2.3 together with one of the split packages from a higher
Symfony vesion like this:
However, you will end up with classes being present twice.
This must be reverted after merging up in the
2.7
branch.