Skip to content
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

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

kbond
Copy link
Member

@kbond kbond commented Mar 29, 2022

This adds a make:scaffold ...<scaffold> command. This is an alternative to what I'm calling single-use makers like make:register/make:user. We could potentially deprecate these other makers.

A scaffold consists of:

  1. files that are copied to your project to create the feature (including tests that are immediately runnable and pass!)
  2. required Composer packages (these are auto-installed)
  3. required JS packages (these are auto-installed)
  4. dependent scaffolds (most current scaffolds depend on others, ie register requires auth)
  5. A little configuration system to allow more advanced file manipulations (ie update an exiting yaml file)

Some additional notes about this feature:

  • Scaffolds are grouped by Symfony major versions with 6.0 being the only supplied version currently.
  • These are meant to be run on new projects. Running them on an established project is possible but since the supplied files override existing files, you'll have to be careful and use git to help with conflicts.
  • Copying files from a scaffold is currently "dumb" and they just replace existing files without confirming. It could be possible to make this smarter.
  • Scaffolds could be grouped into "starter-kits" like what's provided with Laravel Breeze/Jetstream.
  • I don't really want a wizard for these scaffolds. I think these add a huge amount of complexity. If we want a variation on a scaffold, we'd create a new scaffold.
  • A future iteration could provide styling options? tailwind/bootstrap versions of each scaffold
  • The generated code (especially the tests) is opinionated and use several of my personal libraries (zenstruck/browser|foundry). This is really "How Kevin Writes Symfony Apps" :). I don't have an issue if we want to make changes to code/tests to make them "more generic".
  • bin/console make:scaffold (no arguments) lists the available scaffolds to choose from.
  • bin/console make:scaffold x y z installs scaffolds x, y and z.

This PR provides the following scaffolds:

  1. homepage: basic homepage controller, template, functional test for other scaffolds to use
  2. user: user entity/repository, foundry factory and unit test
  3. auth: form login with functional tests
  4. register: registration form with functional tests
  5. change-password: change password form with functional tests
  6. profile: user profile management form with functional tests
  7. reset-password: using symfonycasts/password-reset-bundle
  8. bootstrapcss: installs/configures bootstrap with encore

Included 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:

  • navbar on every page with:
    • login & register links if not logged in
    • manage profile, change password & logout user menu
  • forgot password link on login page

Future ideas for scaffolds:

  • reset-password:stateless: using signed urls instead of db token table
  • register:verification: similar to register but with email verification
  • change-email: change email system for verified emails
  • tailwindcss: similar to the current bootstrapcss scaffold but for Tailwind CSS

Future ideas for starter-kits:

  • starter-kit:verifcation: similar to the current starter kit but with email verification
  • starter-kit:tailwind: similar to the current starter kit but for Tailwind CSS
  • starter-kit:verification:tailwind: combination of the above two starter kits

Test this PR:

symfony new project
cd project
composer config repositories.maker vcs git@github.com:kbond/maker-bundle
composer require symfony/maker-bundle:dev-scaffolding
composer require process # required for this maker
composer require log # (optional) to surpress logs in console

bin/console make:scaffold starter-kit

# run unit tests
DATABASE_URL="sqlite:///%kernel.project_dir%/var/test.db" bin/phpunit

# test in browser!
echo DATABASE_URL="sqlite:///%kernel.project_dir%/var/data.db" > .env.local

symfony server:start -d

Some TODOS:

  • add better descriptions/help for each scaffold, post install notes ("what's next")
  • deprecate some existing maker's that are replaced by scaffolds
  • smarter overwrites: if contents is the same, skip, if contents are different, ask user to confirm.
  • 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.
  • base.html.twig patch with flashes...
  • cleanup configuration system, add some helper methods to FileManager (ie modifyYaml, modifyJson, appendToFile, replaceInFile).
  • suffix the scaffold files with .tpl to avoid your IDE from suggesting
  • instead of using Filesystem::mirror() to copy scaffold files, use FilesystemManager - this will allow the tests to lint the php/template files.
  • replace require package "aliases" with real packages to improve perf

@kbond kbond force-pushed the scaffolding branch 4 times, most recently from bfd3867 to 6272058 Compare March 31, 2022 00:35
@kbond
Copy link
Member Author

kbond commented Mar 31, 2022

Some Symfony/Recipe PR's that would help simplify this:

'property' => 'email',
],
],
];
Copy link
Member

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.

Copy link
Member Author

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/auth.php Outdated Show resolved Hide resolved
@seb-jean
Copy link
Contributor

seb-jean commented Apr 4, 2022

This PR looks really amazing 💪

@jrushlow jrushlow added the Feature New Feature label Apr 5, 2022
@seb-jean
Copy link
Contributor

According to Twig best practices, we should use lower cased and underscored variable names:
https://twig.symfony.com/doc/3.x/coding_standards.html

For instance:

{{ form_start(changePasswordForm) }}

becomes

{{ form_start(change_password_form) }}

And the same in the other templates.

@kbond
Copy link
Member Author

kbond commented Apr 15, 2022

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.

Copy link
Member

@weaverryan weaverryan left a 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/Maker/MakeScaffold.php Outdated Show resolved Hide resolved
src/Resources/scaffolds/6.0/auth.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')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ($this->isGranted('IS_AUTHENTICATED_FULLY')) {
if ($this->isGranted('IS_AUTHENTICATED_REMEMBERED')) {

Copy link
Member Author

@kbond kbond Apr 29, 2022

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')));
Copy link
Member

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?

Copy link
Member Author

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.

@kbond kbond force-pushed the scaffolding branch 2 times, most recently from 47ebada to f1e9503 Compare May 2, 2022 16:19
@@ -0,0 +1 @@
@import "~bootstrap/dist/css/bootstrap.css";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@import "~bootstrap/dist/css/bootstrap.css";
@import "~bootstrap";

Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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();
}
}
Copy link
Member

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).

Copy link
Member Author

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.

Copy link
Member

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!

Copy link
Collaborator

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.

Copy link
Member Author

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}\"?")) {
Copy link
Member

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:');
Copy link
Member

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:');
Copy link
Member

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");
Copy link
Member

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';

Copy link
Member Author

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?

Copy link
Member

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.

@ghost
Copy link

ghost commented May 14, 2022

Would it be worth also using the Undocumented/Only documented in API logout CSRF protection?

logout:
        path: logout
        csrf_parameter: token
        csrf_token_generator: security.csrf.token_manager
<a href="{{ path('logout', {'token' : csrf_token('logout')}) }}">Logout</a>

@kbond
Copy link
Member Author

kbond commented May 14, 2022

Would it be worth also using the Undocumented/Only documented in API logout CSRF protection?

@wouterj?

@wouterj
Copy link
Member

wouterj commented May 14, 2022

Would it be worth also using the Undocumented/Only documented in API logout CSRF protection?

@wouterj?

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 logout_path() helper): symfony/demo#1312 This makes enabling/disabling logout CSRF protection a config-only change, and it's a pretty cool helper overall :)

Copy link
Member

@wouterj wouterj left a 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 and doctrine:migration:migrate.


public function __construct(FileManager $fileManager)
{
$this->executableFinder = new ExecutableFinder();
Copy link
Member

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];
Copy link
Member

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');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$this->denyAccessUnlessGranted('IS_AUTHENTICATED_REMEMBERED');
$this->denyAccessUnlessGranted('IS_AUTHENTICATED');

(see symfony/symfony#42510 for some background)

Comment on lines +39 to +42

public function configureOptions(OptionsResolver $resolver): void
{
}
Copy link
Member

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,
]),
Copy link
Member

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;
Copy link
Member

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
Copy link
Member

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';
Copy link
Member

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.

Copy link
Member Author

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
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Contributor

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];
Copy link
Member

@wouterj wouterj May 15, 2022

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.

@seb-jean
Copy link
Contributor

This PR should be taken into account: #1114 in particular UserRepository.php

new UserPassword(['message' => 'This is not your current password.']),
],
])
->add('plainPassword', PasswordType::class, [
Copy link
Contributor

@seb-jean seb-jean May 30, 2022

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, [
Copy link
Contributor

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,

@seb-jean
Copy link
Contributor

seb-jean commented Jun 8, 2022

What I find good in this PR is the name given to the forms in the view 👍🏻 .

It's not just form, it's for example changePasswordForm, registrationForm, profileForm.

Good continuation for this PR.

@seb-jean
Copy link
Contributor

Would it be interesting to add Form/Model?
For example, the ChangePassword Model:

// 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 ?

@seb-jean
Copy link
Contributor

seb-jean commented Aug 9, 2022

@wouterj

  • The reset password and change password scaffolds are really similar, aren't they? [...]

They look the same except there is an extra field to enter the current password (currentPassword).

@ahmedyakoubi
Copy link

hey @kbond , is this PR going to be ready before the release of symfony 6.2 ?

@kbond
Copy link
Member Author

kbond commented Oct 11, 2022

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants