Make Reset Password Optional #1576
Conversation
|
Could you please rebase your PR and fix merge conflicts? |
|
@VincentLanglet should i try to fix this errors even if i didn't touched this code? |
|
|
|
I made #1595 ; you just need to rebase once 5.x will be in 6.x. Btw, the 6.x release won't be soon. Can't you do the PR on the 5.x ? I don't see a real BC-break |
Hanmac
force-pushed
the
optionalResetting
branch
from
c35a715
to
67c9066
Compare
I'm not very good at documentation and update note stuff i probably need to write something like this:
IMO isn't this still a BC break? and shouldn't that be marked for remove next-major? |
composer.json
Outdated
| "symfony/mailer": "^4.4 || ^5.4 || ^6.0", | ||
| "symfony/mime": "^4.4.10 || ^5.4 || ^6.0", |
There was a problem hiding this comment.
@VincentLanglet i'm unsure, is this a BC Break?
updating with composer would remove the dependency and composer would remove the packages?
which then causes a problem that the current default config (and what the user might have configured)
are still looking for the mailer packages? (i mean it probably would cause problems with cache:clear after composer update, so the user should see the problem directly?)
There was a problem hiding this comment.
Friendly ping @jordisala1991.
I don't remember where but I think we already did such change (which an upgrade note).
The other option would be to keep those dependencies and only remove them in 6.x branch
There was a problem hiding this comment.
Since you have to configure mailer, it should be a dep you have when installing this bundle. IMO we can try moving it to require-dev
| $this->configureDefaultAvatar($config['profile'], $container); | ||
| if (isset($bundles['SonataAdminBundle'])) { | ||
| $this->configureAdmin($config['admin'], $container); | ||
| $this->configureResetting($config['resetting'], $container); | ||
| // check for reset configuration | ||
| if (isset($config['resetting']['email']['address'], $config['resetting']['email']['sender_name'])) { |
There was a problem hiding this comment.
IMO this is missing a check to really validate the user has required deps (since they will be optional on require-dev) wdyt ?
|
WDYT about adding an option to disable reset mail (default to enabled) and deprecate not defining a value for it (like symfony did for the enable_authenticator_manager). Not sure how they did it or how hard is to implement. But would make the feature easier to discover. Also, some docs about how to disable/enable the feature, would be nice. |
There is already which are two values required for reset mail If you don't want reset mail, you remove those value ; and if you want reset mail, you set those values. |
|
Could you please rebase your PR and fix merge conflicts? |
|
Except the need of a rebase, this PR need to update the doc in order to explain how to enable/disable this feature @Hanmac |
Hanmac
force-pushed
the
optionalResetting
branch
from
35cc905
to
8554fc2
Compare
|
Friendly ping @Hanmac ; any time for the doc update ? :) |
|
This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
I still need to do this an some Doc Update Maybe next Weekend |
Thanks. You'll also need a rebase. |
|
This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Let's forget the doc, if you rebase @Hanmac I'll merge it |
Subject
I am targeting this branch, because its BC break (most likely).
Closes #1491.
Changelog
To do