-
-
Notifications
You must be signed in to change notification settings - Fork 895
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
Compatiblity with Symfony 4.3 #2784
Conversation
dunglas
commented
May 10, 2019
•
edited by alanpoulain
Loading
edited by alanpoulain
Q | A |
---|---|
Bug fix? | yes |
New feature? | no |
BC breaks? | no |
Deprecations? | no |
Tests pass? | yes |
Fixed tickets | n/a |
License | MIT |
Doc PR | n/a |
1aa8822
to
58ace00
Compare
4b2af33
to
5f58782
Compare
f140dac
to
04d6f0b
Compare
315509b
to
ccfafca
Compare
Thank you very much @soyuka and @alanpoulain for the help! |
@@ -30,8 +30,6 @@ | |||
/** | |||
* Generic item normalizer. | |||
* | |||
* @final |
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.
I don't think this is right. The GraphQL ItemNormalizer should stop extending this class instead, but instead extend AbstractItemNormalizer
.
@@ -96,10 +97,11 @@ protected function configureContainer(ContainerBuilder $c, LoaderInterface $load | |||
|
|||
$loader->load(__DIR__."/config/config_{$this->getEnvironment()}.yml"); | |||
|
|||
$alg = class_exists(SodiumPasswordEncoder::class) && SodiumPasswordEncoder::isSupported() ? 'auto' : 'bcrypt'; |
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.
Uhh... Symfony really screwed this one up. :x
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.
It's not bcrypt if sodium isn't supported but the native encoder. I do not know if it's important here.
The bcrypt encoder is now deprecated
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.
It's only for old versions so I think we don't care (this code will go be thrown away at some point).
It seems like this is not addressed? #2856 |
I'm not sure to understand how #2856 is an issue here... |
True although I'm not sure to get what we need to do on our side. |
Why only master and not 2.4 branch ? My tests are full of |
Because Messenger. Messenger 4.3 introduces some BC breaks, that in cascade force us to add some BC breaks too. Also, to use Messenger 4.3 we had to bump the version of several other Symfony components. We then decided that it wasn't a good idea to introduce such changes in a patch release. |
We could certainly backport the other changes that are not related to Symfony Messenger. |
Maybe some of them yes! Feel free to open a PR! |
* Compatiblity with Symfony 4.3 * Fix all broken unit tests * Fix some deprecations * Remove @Final from ItemNormalizer * Fix Behat tests * Use composition for the Exception Listener * Fix default firewall logout_on_user_change deprecation * Make sure that sodium is supported * Fix composer and tests * Fix PHPStan config and CS * Remove deprecated option * Fix MongoDB * Fix rebase