Skip to content

[Security] Remove dependency on security-core from security-csrf #16494

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

Closed
wants to merge 1 commit into from

Conversation

enumag
Copy link
Contributor

@enumag enumag commented Nov 7, 2015

Q A
Bug fix? no
New feature? yes
BC breaks? possibly
Deprecations? no
Tests pass? yes
Fixed tickets #15872
License MIT
Doc PR

@Tobion
Copy link
Contributor

Tobion commented Nov 8, 2015

This cannot be done in 2.8 as it's a BC break and also doesn't make sense in 2.8 as one of the main classes (UriSafeTokenGenerator) still depends on Symfony\Component\Security\Core\Util\SecureRandomInterface. But I'm 👍 to make this change in 3.0 as there is deprecated SecureRandomInterface is removed anyway.

@enumag
Copy link
Contributor Author

enumag commented Nov 8, 2015

That's fine by me. If you want me to create new PR against the 3.0 branch let me know.

The change in the exception might cause BC break if somebody was catching it using the parent exception or ExceptionInterface from Securty/Core. Not sure how bad it is to make this change but it would be sad to have a dependency only because of an exception.

@enumag
Copy link
Contributor Author

enumag commented Nov 8, 2015

@Tobion Actually I think the dependency in UriSafeTokenGenerator is not relevant. If the interface doesn't exist, the UriSafeTokenGenerator should still work fine.

@fabpot
Copy link
Member

fabpot commented Jun 16, 2016

Closing as we don't break BC in stable versions and we can't add new classes/exceptions.

@fabpot fabpot closed this Jun 16, 2016
@enumag
Copy link
Contributor Author

enumag commented Sep 26, 2017

@fabpot Would it be possible to do this for 4.0?

@chalasr
Copy link
Member

chalasr commented Sep 26, 2017

@enumag It is for 3.4 (next minor release), could you rebase and update the PR target branch accordingly?

There is no more BC break, reopening.

@chalasr chalasr reopened this Sep 26, 2017
@chalasr chalasr added this to the 3.4 milestone Sep 26, 2017
@Tobion
Copy link
Contributor

Tobion commented Sep 26, 2017

@chalasr why is there no bc break anymore? The exception hierarchy will still be changed in a BC break way.

@nicolas-grekas nicolas-grekas modified the milestones: 3.4, 4.1 Sep 28, 2017
@Hikariii
Copy link

I also think this is in line with the Symfony flex compose all things way of thinking. Even if it breaks a BC exception chain, I can imagine that being acceptable for a 4.* release.

@ostrolucky
Copy link
Contributor

It's still a bc break, so this interface hierarchy change can't be done in 4.*. What can be done in 4.* though is ease the migration. I suggest to not copy these classes over here, just deprecate RuntimeException in security component. In 5 just remove it and stop extending it here. RuntimeException from security is used nowhere else and doesn't bring any value here.

steps left to do here:

  • revert current changes in this PR
  • deprecate \Symfony\Component\Security\Core\Exception\RuntimeException
  • update upgrade.rst

status: needs work

@chalasr
Copy link
Member

chalasr commented Dec 18, 2017

@ostrolucky deprecating would not help here, RuntimeException especially. Triggering a notice when the exception is caught is not possible, and would not be enough anyway as one might catch ExceptionInterface instead.

After all I think it's not worth it, re-closing, sorry for the noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants