[DX][SecurityBundle] UserPasswordEncoderCommand: ask user class choice question #20677

ogizanagi · 2016-11-28T21:40:34Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	N/A
License	MIT
Doc PR	N/A

Typing the user class is quite annoying so I'd suggest to ask the user to select it using a choice question.

This changes however requires to inject configured encoders' user classes. Making the command a service and providing it in the constructor from the extension is probably the cleanest way, but it deprecates:

registering the command by convention (registered as a service now, so potential commands extending this one should do the same)
relying on the fact the command extends ContainerAwareCommand and implements ContainerAwareInterface (will not extends/implements it anymore in 4.0 because it's not required anymore)

ogizanagi · 2016-11-28T22:10:56Z

src/Symfony/Bundle/SecurityBundle/composer.json

@@ -24,7 +24,7 @@
    "require-dev": {
        "symfony/asset": "~2.8|~3.0",
        "symfony/browser-kit": "~2.8|~3.0",
-        "symfony/console": "~2.8|~3.0",
+        "symfony/console": "~3.2",


Required for using CommandTester::setInputs (see https://travis-ci.org/symfony/symfony/jobs/179580451#L3500)

hhamon · 2016-12-01T09:35:39Z

src/Symfony/Bundle/SecurityBundle/Command/UserPasswordEncoderCommand.php

+
+        if (empty($this->userClasses)) {
+            if (null === $this->encoderFactory) {
+                // BC to be remove and simply keep the exception whenever there is no configured user classes in 4.0


s/remove/removed/

hhamon · 2016-12-01T09:36:03Z

src/Symfony/Bundle/SecurityBundle/Command/UserPasswordEncoderCommand.php

+            throw new \RuntimeException('There are no configured encoders for the "security" extension.');
+        }
+
+        if (!$input->isInteractive() || count($this->userClasses) === 1) {


1 === count($this->userClasses)

Yoda conditions +1

javiereguiluz · 2016-12-30T17:00:31Z

I'm sorry that the title of my e99cbe7 commit is so ugly. I just used GitHub interface to test the new feature to resolve conflicts.

javiereguiluz · 2016-12-30T17:01:13Z

👍

Status: reviewed.

@ogizanagi thanks for this DX improvement.

fabpot · 2016-12-31T08:17:04Z

This PR should be rebased as we don't merge PR with merge commits.

ogizanagi · 2016-12-31T12:06:00Z

Sorry @javiereguiluz , I don't know what I should have done eventually in order to keep authorship for the conflict resolution. But anyway, thank you for that.

PR rebased on master.

xabbuh · 2016-12-31T12:13:39Z

The changelog file of the SecurityBundle must be updated too.

ogizanagi · 2016-12-31T12:23:54Z

@xabbuh : Thanks. I've updated it. Let me know how it can possibly be enhanced. :)

xabbuh · 2016-12-31T13:49:23Z

UPGRADE-4.0.md

+SecurityBundle
+--------------
+
+ * The `UserPasswordEncoderCommand` class does not allow `null` as first argument anymore.


[...] as the first [...]

xabbuh · 2016-12-31T13:49:59Z

src/Symfony/Bundle/SecurityBundle/Command/UserPasswordEncoderCommand.php

+    public function __construct(EncoderFactoryInterface $encoderFactory = null, array $userClasses = array())
+    {
+        if (null === $encoderFactory) {
+            @trigger_error(sprintf('Passing null as first argument of "%s" is deprecated since version 3.3 and will be removed in 4.0. If the command was registered by convention, make it a service instead.', __METHOD__), E_USER_DEPRECATED);


[...] as the first [...]

xabbuh · 2016-12-31T13:51:09Z

UPGRADE-4.0.md

+
+ * The `UserPasswordEncoderCommand` class does not allow `null` as first argument anymore.
+
+ * `UserPasswordEncoderCommand` does not extend `ContainerAwareCommand` nor implement 


Not sure we need to talk about the class. Not implementing the interface implies that the command cannot be an instance of ContainerAwareCommand anymore.

xabbuh · 2016-12-31T13:51:46Z

src/Symfony/Bundle/SecurityBundle/CHANGELOG.md

+3.3.0
+-----
+
+ * Deprecated registering by convention the `UserPasswordEncoderCommand` command 


Is this deprecation necessary? The command will be registered by the SecurityBundle anyway, won't it?

It's more about extending the command and registering it from another bundle actually. Can be rephrased I guess.

ogizanagi · 2017-01-15T20:27:28Z

I think this one is ready :)

javiereguiluz · 2017-01-16T07:46:44Z

@ogizanagi I've tested this and it works nice! Thanks.

I'm not sure this idea belongs to this PR, but the list of user classes maybe could be sorted alphabetically to make it easier to find them. At the moment the are displayed in the same order as defined in the security file:

ogizanagi · 2017-01-16T08:42:57Z

@javiereguiluz : That's a nice suggestion indeed. I've updated the code to use natcasesort. :)

HeahDude

👍

HeahDude · 2017-01-19T18:53:03Z

src/Symfony/Bundle/SecurityBundle/Command/UserPasswordEncoderCommand.php

+            return reset($this->userClasses);
+        }
+
+        $userClasses = $this->userClasses;


$userClasses = array_values($this->userClasses); natcasesort($userClasses);

?

natcasesort preserves keys. Hence the array_values call afterwards :)

fabpot · 2017-02-16T14:24:34Z

src/Symfony/Bundle/SecurityBundle/Command/UserPasswordEncoderCommand.php

    /**
     * {@inheritdoc}
     */
    protected function configure()
    {
        $this
-            ->setName('security:encode-password')


Should be kept and the call in __construct changed to: parent::__construct(null);

Fixed, thanks.

fabpot · 2017-02-16T14:24:56Z

src/Symfony/Bundle/SecurityBundle/Command/UserPasswordEncoderCommand.php

-If you execute the command non-interactively, the default Symfony User class
-is used and a random salt is generated to encode the password:
+If you execute the command non-interactively, the first available configured user class under 
+the security.encoders key is used and a random salt is generated to encode the password:


security.encoders could be wrapped with a <comment> tag

fabpot · 2017-02-16T14:48:34Z

src/Symfony/Bundle/SecurityBundle/Command/UserPasswordEncoderCommand.php

-If you execute the command non-interactively, the default Symfony User class
-is used and a random salt is generated to encode the password:
+If you execute the command non-interactively, the first available configured 
+user class under the <comment>security.encoders</comment> key is used and a 


You should split lines at 80 chars (excluding the tags)

…estion

fabpot · 2017-02-16T15:35:57Z

Thank you @ogizanagi.

…ser class choice question (ogizanagi) This PR was merged into the 3.3-dev branch. Discussion ---------- [DX][SecurityBundle] UserPasswordEncoderCommand: ask user class choice question | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | N/A Typing the user class is quite annoying so I'd suggest to ask the user to select it using a choice question. This changes however requires to inject configured encoders' user classes. Making the command a service and providing it in the constructor from the extension is probably the cleanest way, but it deprecates: - registering the command by convention (registered as a service now, so potential commands extending this one should do the same) - relying on the fact the command extends `ContainerAwareCommand` and implements `ContainerAwareInterface` (will not extends/implements it anymore in 4.0 because it's not required anymore) Commits ------- 366aefd [SecurityBundle] UserPasswordEncoderCommand: ask user class choice question

carsonbot added Status: Needs Review SecurityBundle Feature Deprecation labels Nov 28, 2016

ogizanagi commented Nov 28, 2016

View reviewed changes

ogizanagi changed the title ~~[SecurityBundle] UserPasswordEncoderCommand: ask user class choice question~~ [DX][SecurityBundle] UserPasswordEncoderCommand: ask user class choice question Nov 29, 2016

hhamon reviewed Dec 1, 2016

View reviewed changes

nicolas-grekas added this to the 3.x milestone Dec 6, 2016

javiereguiluz added the DX DX = Developer eXperience (anything that improves the experience of using Symfony) label Dec 30, 2016

javiereguiluz added Status: Reviewed and removed Status: Needs Review labels Dec 30, 2016

xabbuh reviewed Dec 31, 2016

View reviewed changes

HeahDude approved these changes Jan 19, 2017

View reviewed changes

fabpot approved these changes Feb 16, 2017

View reviewed changes

fabpot reviewed Feb 16, 2017

View reviewed changes

[SecurityBundle] UserPasswordEncoderCommand: ask user class choice qu…

366aefd

…estion

fabpot merged commit 366aefd into symfony:master Feb 16, 2017

ogizanagi deleted the feature/security_bundle/enc_pwd_select_user_class branch February 16, 2017 15:36

javiereguiluz mentioned this pull request Feb 26, 2017

Simplified UserPasswordEncoderCommand symfony/symfony-docs#7539

Closed

nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

weaverryan mentioned this pull request May 22, 2017

[SecurityBundle] Prevent auto-registration of UserPasswordEncoderCommand #22858

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DX][SecurityBundle] UserPasswordEncoderCommand: ask user class choice question #20677

[DX][SecurityBundle] UserPasswordEncoderCommand: ask user class choice question #20677

ogizanagi commented Nov 28, 2016 •

edited

Loading

ogizanagi Nov 28, 2016

hhamon Dec 1, 2016

hhamon Dec 1, 2016

roberto910907 Apr 20, 2017

javiereguiluz commented Dec 30, 2016

javiereguiluz commented Dec 30, 2016

fabpot commented Dec 31, 2016

ogizanagi commented Dec 31, 2016

xabbuh commented Dec 31, 2016

ogizanagi commented Dec 31, 2016

xabbuh Dec 31, 2016

xabbuh Dec 31, 2016

xabbuh Dec 31, 2016

xabbuh Dec 31, 2016

ogizanagi Dec 31, 2016

ogizanagi commented Jan 15, 2017

javiereguiluz commented Jan 16, 2017

ogizanagi commented Jan 16, 2017

HeahDude left a comment

HeahDude Jan 19, 2017

ogizanagi Jan 19, 2017

fabpot Feb 16, 2017

ogizanagi Feb 16, 2017

fabpot Feb 16, 2017

fabpot Feb 16, 2017

ogizanagi Feb 16, 2017

fabpot commented Feb 16, 2017


		* The `UserPasswordEncoderCommand` class does not allow `null` as first argument anymore.

		* `UserPasswordEncoderCommand` does not extend `ContainerAwareCommand` nor implement

[DX][SecurityBundle] UserPasswordEncoderCommand: ask user class choice question #20677

[DX][SecurityBundle] UserPasswordEncoderCommand: ask user class choice question #20677

Conversation

ogizanagi commented Nov 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiereguiluz commented Dec 30, 2016

javiereguiluz commented Dec 30, 2016

fabpot commented Dec 31, 2016

ogizanagi commented Dec 31, 2016

xabbuh commented Dec 31, 2016

ogizanagi commented Dec 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogizanagi commented Jan 15, 2017

javiereguiluz commented Jan 16, 2017

ogizanagi commented Jan 16, 2017

HeahDude left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Feb 16, 2017

ogizanagi commented Nov 28, 2016 •

edited

Loading