-
-
Notifications
You must be signed in to change notification settings - Fork 404
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
[RFC][WIP] add scaffolding system (make:scaffold
)
#1085
base: main
Are you sure you want to change the base?
Conversation
bfd3867
to
6272058
Compare
Some Symfony/Recipe PR's that would help simplify this:
|
src/Resources/scaffolds/6.0/reset-password/tests/Functional/ResetPasswordTest.php
Outdated
Show resolved
Hide resolved
src/Resources/scaffolds/6.0/reset-password/tests/Functional/ResetPasswordTest.php
Outdated
Show resolved
Hide resolved
src/Resources/scaffolds/6.0/reset-password/tests/Functional/ResetPasswordTest.php
Outdated
Show resolved
Hide resolved
src/Resources/scaffolds/6.0/auth.php
Outdated
'property' => 'email', | ||
], | ||
], | ||
]; |
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 seems like this change should go into the "user" scaffold, no? We currently add this in make:user
.
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 struggled with this a bit. Don't really need it in user because you haven't made the decision yet on how it will be authenticated...
src/Resources/scaffolds/6.0/change-password/tests/Functional/User/ChangePasswordTest.php
Outdated
Show resolved
Hide resolved
src/Resources/scaffolds/6.0/profile/tests/Functional/User/ProfileTest.php
Outdated
Show resolved
Hide resolved
src/Resources/scaffolds/6.0/profile/tests/Functional/User/ProfileTest.php
Outdated
Show resolved
Hide resolved
src/Resources/scaffolds/6.0/register/tests/Functional/User/RegisterTest.php
Outdated
Show resolved
Hide resolved
src/Resources/scaffolds/6.0/register/tests/Functional/User/RegisterTest.php
Outdated
Show resolved
Hide resolved
This PR looks really amazing 💪 |
According to Twig best practices, we should For instance: {{ form_start(changePasswordForm) }} becomes {{ form_start(change_password_form) }} And the same in the other templates. |
Hard to tell but my interpretation of that is for variables set in the template. The templates are actually run through twigcs as part of the tests and I fixed several issues yesterday. |
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.
Also can we batch the "composer requires" together (at least within a single scaffold)? Same with adding JS packages
Thanks for working on this - it's super fun (especially when you style with the bootstrapcss
scaffold).
src/Resources/scaffolds/6.0/reset-password/tests/Functional/ResetPasswordTest.php
Outdated
Show resolved
Hide resolved
#[Route(path: '/login', name: 'login')] | ||
public function login(AuthenticationUtils $authenticationUtils, Request $request): Response | ||
{ | ||
if ($this->isGranted('IS_AUTHENTICATED_FULLY')) { |
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.
if ($this->isGranted('IS_AUTHENTICATED_FULLY')) { | |
if ($this->isGranted('IS_AUTHENTICATED_REMEMBERED')) { |
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.
Don't think this is right. We want a way to fully authenticate only remembered users. An example would be going to your billing settings: this requires IS_AUTHENTICATED_FULLY
, if your are only IS_AUTHENTICATED_REMEMBERED
, you want to send the user to the login screen to fully authenticate.
public function login(AuthenticationUtils $authenticationUtils, Request $request): Response | ||
{ | ||
if ($this->isGranted('IS_AUTHENTICATED_FULLY')) { | ||
return new RedirectResponse($request->query->get('target', $this->generateUrl('homepage'))); |
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.
This target
things is referenced in the template also. What is it?
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 allows creating login links on every page (ie in the navbar) that will redirect back to the page the user was on when clicking login. Is there a better way to do this? I don't mind dropping this either.
src/Resources/scaffolds/6.0/reset-password/src/Controller/ResetPasswordController.php.tpl
Outdated
Show resolved
Hide resolved
src/Resources/scaffolds/6.0/reset-password/src/Entity/ResetPasswordRequest.php.tpl
Outdated
Show resolved
Hide resolved
src/Resources/scaffolds/6.0/reset-password/src/Entity/ResetPasswordRequest.php.tpl
Outdated
Show resolved
Hide resolved
src/Resources/scaffolds/6.0/reset-password/src/Form/ResetPasswordFormType.php.tpl
Outdated
Show resolved
Hide resolved
src/Resources/scaffolds/6.0/reset-password/src/Form/ResetPasswordFormType.php.tpl
Outdated
Show resolved
Hide resolved
47ebada
to
f1e9503
Compare
@@ -0,0 +1 @@ | |||
@import "~bootstrap/dist/css/bootstrap.css"; |
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.
@import "~bootstrap/dist/css/bootstrap.css"; | |
@import "~bootstrap"; |
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.
This is a spot where, like with app.js
, I'd love to patch instead of replace. There's a practical reason. Right now, when you run the bootstrapcss scaffold, (A) the recipe adds bootstrap.css
and then (B) the scaffold tries to replace it... so it immediately asks us if we want to overwrite app.css
... which is weird :p.
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.
What should we do about the default background color added by the recipe?
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 was wondering that too. Ignore it? As I, keep it there (and just add the import for bootstrap). It doesn't really hurt anything. I added that background just so there was SOMETHING, and then you'd think "hey, where does that come from". I think it still accomplishes that.
if ($flush) { | ||
$this->_em->flush(); | ||
} | ||
} |
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 think add()
and remove()
are too opinionated. It doesn't add any real functionality, but makes the code harder to read (if these methods were a Symfony standard, I'd have a different opinion, but since they're not, I think it adds misdirection: it's harder for people to understand the generated code).
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.
What do you suggest here? I copied these methods from the make:entity
maker.
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.
oooooh, I forgot we added those. Nevermind then!
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.
In #1087 those methods do not flush()
by default anymore.
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.
In #1087 those methods do not flush() by default anymore.
Ok, I'll change to match.
foreach (Finder::create()->files()->in($scaffold['dir']) as $file) { | ||
$filename = "{$file->getRelativePath()}/{$file->getFilenameWithoutExtension()}"; | ||
|
||
if (!$this->fileManager->fileExists($filename) || $io->confirm("Overwrite \"{$filename}\"?")) { |
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.
Let's make this smarter: if the target file and proposed file contents are IDENTICAL, simply skip writing to the file or asking this question.
Also, when I did get this question legitimately, I wasn't sure what to do. What changes did it want to make? How about:
Overwrite "assets/styles/app.css"? (yes/no/review) [yes]:
And if the user chooses "review", we show them what the NEW file would look like (showing a diff would be awesome... but I'm not sure that's possible unless we decide to add a dependency like https://github.com/jfcherng/php-diff).
$io->newLine(); | ||
|
||
if ($scaffold['dependents'] ?? []) { | ||
$io->text('This scaffold will also install the following scaffolds:'); |
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.
If we're going to show this... which I think WAS nice... then it should be recursive. For example, iirc, I did register
, which reported that it was going to also install auth
... then the first scaffold it actually did was user
.
} | ||
|
||
if ($scaffold['packages'] ?? []) { | ||
$io->text('This scaffold will install the following composer packages:'); |
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.
Same here: if we're going to show this, it should be recursive.
Also, I think this lists packages that I already have installed? Ideally, it would skip those: tell me what will really be installed.
return; | ||
} | ||
|
||
$files->dumpFile('assets/app.js', $appJs."require('bootstrap');\n"); |
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'm thinking:
// activates all of Bootstrap's JavaScrip functionality
import 'bootstrap';
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.
Does import here have the same effect as require?
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 does, I tested it locally to be absolutely sure.
Would it be worth also using the Undocumented/Only documented in API logout CSRF protection?
|
|
Strictly speaken, I think you can get away with only having CSRF on login (or only on log-out). But it doesn't hurt that much to enable logout CSRF as well (and it keeps applications secure when they move away from the default form_login config provided by the scaffold). I would recommend using the approach that I submitted to the demo application (using the |
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.
Hi! Thank you for all the work on this PR. It's a great DX to see how quickly you can get a working system using these scaffolds, and being static the scaffolds also appear easier to maintain in this bundle :)
I've been testing this a bit in a new project, and left some comments on the scaffold code (mostly with some optional tips and tricks that we might want to apply). Besides this, I also have two more high-level questions:
- The reset password and change password scaffolds are really similar, aren't they? Do we want to provide these almost similar scaffolds, or do we rather want to use the SymfonyCastsPasswordResetBundle on the profile edit page as well? (easier to maintain & less chance of introducing vulnerabilities)
- Should the scaffolds create & run database migrations? (with a confirmation question) I think we should either do this automatically, or add a message with "next steps" that show
make:migrate
anddoctrine:migration:migrate
.
|
||
public function __construct(FileManager $fileManager) | ||
{ | ||
$this->executableFinder = new ExecutableFinder(); |
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.
symfony/process
is only a dev requirement of MakerBundle at the moment. After installing a fresh Symfony project with this PR's MakerBundle, I get this error when running make:scaffold
:
Attempted to load class "ExecutableFinder" from namespace "Symfony\Component\Process".\n
Did you forget a "use" statement for another namespace?
if (!$this->isPackageInstalled($package)) { | ||
$io->text("Installing Composer package: <comment>{$package}</comment>..."); | ||
|
||
$command = [$this->composerBin(), 'require', '--no-scripts', 'dev' === $env ? '--dev' : null, $package]; |
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.
Imho, we should always explicitly version the dependent packages. When Symfony 7 is released, requiring e.g. symfony/security-bundle
will install 7.0 - even in a 6.0 scaffold. I don't think this is what we want as it'll lead to an immediate fatal error from the Flex SYMFONY_REQUIRE feature.
UserPasswordHasherInterface $userPasswordHasher, | ||
UserRepository $userRepository, | ||
): Response { | ||
$this->denyAccessUnlessGranted('IS_AUTHENTICATED_REMEMBERED'); |
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.
$this->denyAccessUnlessGranted('IS_AUTHENTICATED_REMEMBERED'); | |
$this->denyAccessUnlessGranted('IS_AUTHENTICATED'); |
(see symfony/symfony#42510 for some background)
|
||
public function configureOptions(OptionsResolver $resolver): void | ||
{ | ||
} |
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.
Is there a specific reason to implement this empty method? (it's also empty in AbstractType
)
If it's here for visibility, I think it's a good idea to add a comment in the method body. Otherwise, I would remove it.
'minMessage' => 'Your password should be at least {{ limit }} characters', | ||
// max length allowed by Symfony for security reasons | ||
'max' => 4096, | ||
]), |
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.
Do we want to showcase the NotCompromisedPassword
constraint here?
if (!$io->confirm("Would your like to create the \"{$name}\" scaffold?")) { | ||
$io->text('Going back to main menu...'); | ||
|
||
continue; |
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.
Currently, there is no way to exit this loop without running a scaffold (or using Ctrl+C). What about allowing "quit" or a blank answer in the choice?
use Symfony\Component\Validator\Constraints\Email; | ||
use Symfony\Component\Validator\Constraints\NotBlank; | ||
|
||
class RequestResetFormType extends AbstractType |
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.
RequestPasswordResetFormType ?
'configure' => function (FileManager $files) { | ||
// add LoginUser service | ||
$files->manipulateYaml('config/services.yaml', function (array $data) { | ||
$data['services']['App\Security\LoginUser']['$authenticator'] = '@security.authenticator.form_login.main'; |
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 know it's a bit hard to accomplish (or maybe even impossible), but in the current state this results in the following services.yaml
file:
services:
# default configuration for services in *this* file
_defaults:
autowire: true # Automatically injects dependencies in your services.
autoconfigure: true # Automatically registers your services as commands, event subscribers, etc.
# makes classes in src/ available to be used as services
# this creates a service per class whose id is the fully-qualified class name
App\:
resource: '../src/'
exclude:
- '../src/DependencyInjection/'
- '../src/Entity/'
- '../src/Kernel.php'
App\Security\LoginUser:
$authenticator: '@security.authenticator.form_login.main'
# add more service definitions when explicit configuration is needed
# please note that last definitions always *replace* previous ones
It would be cool if we can somehow get this definition after the last comment instead.
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'm sort of thinking to make the first scaffolds 6.1+ - this way we can use the Autowire
parameter attribute and remove the need to modify this yaml file.
|
||
#[ORM\Entity(repositoryClass: UserRepository::class)] | ||
#[UniqueEntity(fields: ['email'], message: 'There is already an account with this email')] | ||
class User implements UserInterface, PasswordAuthenticatedUserInterface |
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.
user
is a reserved SQL keyword and needs to be escaped in postgres like "user"
. Given Postgres is the standard set-up when using Symfony's Flex recipes, I think code generated by this bundle must be compatible with it.
This means we would have to add #[ORM\Table(name: '"user"')]
. This is supported by MySQL and Postgres, but breaks compatibility with MariaDB - which does not support double quotes for escaping. On the other hand, Postgres does not support backtick escaping from MariaDB/MySQL.
Probably the best is to use a different table name, do you have any good alternatives?
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.
What about users
? I typically pluralize my table names.
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.
Lets please go with the pluralized version 👍
if (!$this->isPackageInstalled($package)) { | ||
$io->text("Installing Composer package: <comment>{$package}</comment>..."); | ||
|
||
$command = [$this->composerBin(), 'require', '--no-scripts', 'dev' === $env ? '--dev' : null, $package]; |
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 think we should require packages using --no-update
and do a composer update package1 package2 ...
after the loop. Doing a single update will speed up the process for slower internet connections.
This PR should be taken into account: #1114 in particular |
new UserPassword(['message' => 'This is not your current password.']), | ||
], | ||
]) | ||
->add('plainPassword', PasswordType::class, [ |
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.
Change PasswordType::class
to RepeatedType::class
?
And add 'type' => PasswordType::class,
public function buildForm(FormBuilderInterface $builder, array $options): void | ||
{ | ||
$builder | ||
->add('plainPassword', PasswordType::class, [ |
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.
Change PasswordType::class
to RepeatedType::class
?
And add 'type' => PasswordType::class,
What I find good in this PR is the name given to the forms in the view 👍🏻 . It's not just Good continuation for this PR. |
Would it be interesting to add // src/Form/Model/ChangePassword.php
namespace App\Form\Model;
use Symfony\Component\Security\Core\Validator\Constraints as SecurityAssert;
class ChangePassword
{
#[SecurityAssert\UserPassword(
message: 'This is not your current password.',
)]
protected $currentPassword;
[...]
} The idea is to put the validation constraints in the Model and remove it in the Form classes. The Form class would then be easier to read. It's better to organize. What do you think ? |
They look the same except there is an extra field to enter the current password ( |
hey @kbond , is this PR going to be ready before the release of symfony 6.2 ? |
Hey, no, I don't think so. It's a bit stalled right now as we've been discussing the possibility of making this feature part of flex (the recipe system) itself. I'm hoping to nail down what direction we want to go at SymfonyCon (in Novemeber). |
This adds a
make:scaffold ...<scaffold>
command. This is an alternative to what I'm calling single-use makers likemake:register
/make:user
. We could potentially deprecate these other makers.A scaffold consists of:
register
requiresauth
)Some additional notes about this feature:
bin/console make:scaffold
(no arguments) lists the available scaffolds to choose from.bin/console make:scaffold x y z
installs scaffoldsx
,y
andz
.This PR provides the following scaffolds:
homepage
: basic homepage controller, template, functional test for other scaffolds to useuser
: user entity/repository, foundry factory and unit testauth
: form login with functional testsregister
: registration form with functional testschange-password
: change password form with functional testsprofile
: user profile management form with functional testsreset-password
: using symfonycasts/password-reset-bundlebootstrapcss
: installs/configures bootstrap with encoreIncluded is also a
starter-kit
. This is just another scaffold but it includes all the above scaffolds (styled with bootstrap) and an application shell. The shell has the following features:Future ideas for scaffolds:
reset-password:stateless
: using signed urls instead of db token tableregister:verification
: similar to register but with email verificationchange-email
: change email system for verified emailstailwindcss
: similar to the current bootstrapcss scaffold but for Tailwind CSSFuture ideas for starter-kits:
starter-kit:verifcation
: similar to the current starter kit but with email verificationstarter-kit:tailwind
: similar to the current starter kit but for Tailwind CSSstarter-kit:verification:tailwind
: combination of the above two starter kitsTest this PR:
Some TODOS:
Adjust password-reset to flash the "email sent" message instead of a separate template.Decided to leave the email sent template as there is more detail than we'd probably want in a flash message.FileManager
(iemodifyYaml
,modifyJson
,appendToFile
,replaceInFile
)..tpl
to avoid your IDE from suggestingFilesystem::mirror()
to copy scaffold files, useFilesystemManager
- this will allow the tests to lint the php/template files.