[make:security:json-login] WIP #1246
Conversation
jrushlow
force-pushed
the
feature/make-security-json-login
branch
from
4fe922a to
478077d
Compare
| '@Symfony' => true, | ||
| '@Symfony:risky' => true, | ||
| 'native_function_invocation' => false, | ||
| 'blank_line_before_statement' => ['statements' => ['break', 'case', 'continue', 'declare', 'default', 'do', 'exit', 'for', 'foreach', 'goto', 'if', 'include', 'include_once', 'phpdoc', 'require', 'require_once', 'return', 'switch', 'throw', 'try', 'while', 'yield', 'yield_from']], |
There was a problem hiding this comment.
we need phpdoc to be added... TODO: check whats included in symfony rules set and only add the one we need.
There was a problem hiding this comment.
im using our test config here because, well lazy is cool, but thinking we either duplicate this for generating templates or rename / move this away from "test" and still use it for both ci and generating templates. See CI failures related to other makers as a result of the added rule.
jrushlow
force-pushed
the
feature/make-security-json-login
branch
from
a692b79 to
8432cf1
Compare
|
|
||
| // last username entered by the user | ||
| /** last username entered by the user */ | ||
| $lastUsername = $authenticationUtils->getLastUsername(); |
There was a problem hiding this comment.
php-cs-fixer does not have the capability to add a space above a comments (//) but it can do it with docblocks (/** */)
Without changing the comments over, we would end up with:
// get the login error if there is one
$error = $authenticationUtils->getLastAuthenticationError();
// last username entered by the user
$lastUsername = $authenticationUtils->getLastUsername();which could be alright?
| $this->userNameField = $securityHelper->guessUserNameField($io, $this->userClass, $securityData['security']['providers']); | ||
| $this->willLogout = $io->confirm('Do you want to generate a \'/logout\' URL?'); | ||
| } | ||
| } |
There was a problem hiding this comment.
We could probably find more repeated code between make:json & make:form logins if we looked hard enough...
| 'Choose a name for the controller class (e.g. <fg=yellow>ApiLoginController</>)', | ||
| 'ApiLoginController', | ||
| [Validator::class, 'validateClassName'] | ||
| ); |
There was a problem hiding this comment.
This question and the suggestion of ApiLoginController are in the abstract security maker?
| public function configureDependencies(DependencyBuilder $dependencies): void | ||
| { | ||
| $dependencies->addClassDependency(SecurityBundle::class, 'security'); | ||
| $dependencies->addClassDependency(Process::class, 'process'); |
| 'use_statements' => $useStatements, | ||
| 'controller_name' => $controllerNameDetails->getShortName(), | ||
| ] | ||
| ); |
There was a problem hiding this comment.
This could be a new generateEmptyController() on Generator 🤔
| $this->writeSuccessMessage($io); | ||
|
|
||
| $io->text([ | ||
| 'Next: Make a <info>POST</info> request to <info>/api/login</info> with a <info>username</info> and <info>password</info> to login.', |
There was a problem hiding this comment.
The user also needs to set Content-Type: application/json iirc. This is sometimes a "gotcha", so it would be nice to mention it here.
| @@ -0,0 +1,11 @@ | |||
| <?php | |||
There was a problem hiding this comment.
The _L (capital L) looks weird, but I can see you did that on purpose. What was the reason?
|
|
||
| $this->addUseStatements($manipulator, [Route::class]); | ||
|
|
||
| $methodBuilder->addParam((new Param('authenticationUtils'))->setType('AuthenticationUtils')); |
There was a problem hiding this comment.
Will there already be a use statement for this? In addLoginMethod(), we explicitly add the use statement.
| (new Param('user')) | ||
| ->setType(new NullableType($userClass->getShortName())) | ||
| ->addAttribute(new Attribute(new Name('CurrentUser'))) | ||
| ); |
There was a problem hiding this comment.
This probably needs to change to $this->getUser() for better "portability" across all versions of Symfony, DoctrineBundle, FWEXtra, etc?
There was a problem hiding this comment.
Should we bother making that conditional based on the Symfony Version? Or perhaps we do that (CurrentUser) later when say 6.4 or 7 come out for less of a headache for us
jrushlow
force-pushed
the
feature/make-security-json-login
branch
from
96cc0d3 to
df01550
Compare
2 participants
No description provided.