MakeAuthenticator : update security.yaml #261
Conversation
There was a problem hiding this comment.
Hey @nikophil!
GREAT work! I left comments - a lot of minor stuff. You handled the flow, questions, edge-cases and tests very well. Super excited about this!
src/Maker/MakeAuthenticator.php
Outdated
| { | ||
| $fs = new Filesystem(); | ||
|
|
||
| if (!$fs->exists($path = 'config/packages/security.yaml')) { |
There was a problem hiding this comment.
Use $this->fileManager->fileExists() instead - you already did this below in generate() :)
src/Maker/MakeAuthenticator.php
Outdated
| $interactiveSecurityHelper = new InteractiveSecurityHelper(); | ||
|
|
||
| $command->addOption('firewall-name', null, InputOption::VALUE_OPTIONAL, ''); | ||
| $interactiveSecurityHelper->guessFirewallName($input, $io, $securityData); |
There was a problem hiding this comment.
I think guessFirewallName() and guessEntryPoint() should both return the firewall name & entry point, instead of setting it onto the option. It's just a bit more clear.
src/Maker/MakeAuthenticator.php
Outdated
| ]); | ||
| $text = ['Next: Customize your new authenticator.']; | ||
| if (!$securityYamlUpdated) { | ||
| $text[] = 'Then, configure the "guard" key on your firewall to use it.'; |
There was a problem hiding this comment.
Good job adding a message in this case. Let's make it even more explicit (from MakeUser):
$text[] = "Your <info>security.yaml</info> could not be updated automatically. You'll need to add the following config manually:\n\n".$yamlExample;Where $yamlExample is a config dump of the stuff they need to add.
| */ | ||
| public function guessFirewallName(InputInterface $input, SymfonyStyle $io, array $securityData) | ||
| { | ||
| $firewalls = array_filter( |
tests/Maker/FunctionalTest.php
Outdated
| $finder = new Finder(); | ||
| $finder->in($directory.'/src/Security') | ||
| ->name('AppCustomAuthenticator.php'); | ||
| $this->assertCount(1, $finder); |
There was a problem hiding this comment.
I think we can avoid this check on all the tests, except for the frist.
tests/Maker/FunctionalTest.php
Outdated
| [ | ||
| // class name | ||
| 'AppCustomAuthenticator', | ||
| 1 |
There was a problem hiding this comment.
same here - comment for what the option is
tests/Maker/FunctionalTest.php
Outdated
| // class name | ||
| 'AppCustomAuthenticator', | ||
| 1, | ||
| 1 |
| * | ||
| * @param array $securityData | ||
| * @param string $expectedFirewallName | ||
| * @param bool $multipleValues |
| $input = $this->createMock(InputInterface::class); | ||
| $input->expects($this->once()) | ||
| ->method('setOption') | ||
| ->with('firewall-name', $expectedFirewallName); |
There was a problem hiding this comment.
This will simplify a but, when we start passing in $firewallName directly :)
| */ | ||
| final class InteractiveSecurityHelper | ||
| { | ||
| public function guessFirewallName(SymfonyStyle $io, array $securityData) |
|
Test failures are unrelated - working on them in a different PR :) |
|
This is awesome! Thank you Nicolas! |
This PR was squashed before being merged into the 1.0-dev branch (closes #261). Discussion ---------- MakeAuthenticator : update security.yaml This PR adds the newly created authenticator to `security.yaml` The process is the following : - if no firewall exist, a "main" firewall is automatically created (this should never happen) - if there is only one "real" firewall (i.e. : a firewall withou security: false"), the authenticator is added to this firewall - if there are more than one firewall, the user is asked to which firewall the authenticator should be added - if the target firewall already has an authenticator, the user is asked which authenticator should be the entry point. Commits ------- 3a26d98 add return type to InteractiveSecurityHelper::guessFirewallName a983caa fixes after PR 2186549 Add entry point if one or more authenticators exist on chosen firewall c63a86a Add authenticator : let the user chose which firewall to update 2fb5b40 add authenticator in security.yaml the simplest way
This PR adds the newly created authenticator to
security.yamlThe process is the following :