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

Add make:crud & improve make:form commands #113

Merged
merged 52 commits into from
Mar 14, 2018
Merged

Conversation

sadikoff
Copy link
Contributor

@sadikoff sadikoff commented Jan 25, 2018

Hi all,

Improved make:form command

Main improvement is that command works in two ways. First way simple, it just generates skeleton form class and user can do with it whatever he want. Second way is generation from entity class. Generator checks if entity exists and ask user if he wants to generate form from entity or user can use --entity flag if he knows that entity exists.

Generation features:

  • Simple skeleton form class
  • Doctrine entity form class

Examples:

Simple class generation

root@striX: ~/project_root# bin/console make:form

 The name of the form class (e.g. GentleElephantType):
 > SimpleForm

Enter the class or entity name that the new form will be bound to (empty for none):
 >

 created: src/Form/SimpleFormType.php


  Success!

Entity form generation

root@striX: ~/project_root# bin/console make:form 

 The name of the form class (e.g. GentleElephantType):
 > EntityFormType

Enter the class or entity name that the new form will be bound to (empty for none):
 > Entity

 created: src/Form/EntityFormType.php


  Success!

Closes: #114

New make:crud command

Want to introduce make:crud command it is similar to doctrine:generate:crud from SensioGeneratorBundle.

All you need to generate cool crud is Entity :)

Features

  • complete crud generation

Example:

root@striX: ~/project_root# bin/console make:crud SweetFood

 created: src/Controller/SweetFoodController.php
 created: src/Form/SweetFoodType.php
 created: templates/sweet_food/_delete_form.html.twig
 created: templates/sweet_food/_form.html.twig
 created: templates/sweet_food/index.html.twig
 created: templates/sweet_food/show.html.twig
 created: templates/sweet_food/new.html.twig
 created: templates/sweet_food/edit.html.twig


  Success!

Closes: #3

@komujohn
Copy link

good work @sadikoff just what I needed

@sblondeau
Copy link

Thanks @sadikoff for this great work. However it seems to be exactly the same crud as in sf3, with the same drawbacks, like the ugly html code with ul/li, the unuseful "action" col in the index table (a link on the id should be much more convenient ...).
I know it is impossible to satisfy everybody with this kind of generator but I think that the sf4 crud should be an enhancement of sf3 version, not just the same. ;-)

@YannickFricke
Copy link

@sblondeau But what about custom designs?
I would not like to have prewritten CSS for that.
Maybe there should be an option for the command (but please set the standard to false).

@sadikoff
Copy link
Contributor Author

@sblondeau I agree that we need some enhancements. May be we can discuss it?

@sadikoff
Copy link
Contributor Author

I have a question. Is it right to inject EntityManager to maker? or may be there is another way to get Entity metadata?


{% block body %}
<h1><?= $entity_class_name; ?> index</h1>
<table>
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add thead and tbody tags? They seem to be pretty popular these days.

Copy link
Member

Choose a reason for hiding this comment

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

Haha, I agree - they're super hipster :)

{% block body %}
<h1>Create new <?= $entity_class_name; ?></h1>
{{ form_start(form) }}
{{ form_widget(form) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

<?php foreach ($entity_fields as $field): ?>{{ form_row(form.<?= ucfirst($field['fieldName']); ?>) }}
<?php endforeach; ?>

would be my preference here, and I would use a seperate _form.html.twig to generate the same form for the two occurrences. https://github.com/symfony/demo/blob/master/templates/admin/blog/_form.html.twig

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we really need to generate whole form with all fields separated?

*
* @return \Symfony\Component\Form\FormInterface The form
*/
private function createDeleteForm(<?= $entity_class_name; ?> $<?= $entity_var_singular; ?>)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe follow the Symfony demo in this case by using a seperate template to generate a delete form? https://github.com/symfony/demo/blob/master/templates/admin/blog/_delete_form.html.twig

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a big question with delete form for me. May be there is a way to create a delete link in order to delete entities from list and more customization in other templates? Delete form is a good and secure way, but we can include it only in edit or show action.

Copy link
Contributor

@codedmonkey codedmonkey Jan 30, 2018

Choose a reason for hiding this comment

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

With a seperate template this can be achieved with

{% include '.../_delete_form.html.twig with {'post': post.id} %}  

but I don't know whether there are security concerns when using multiple forms with the same CSRF token.

<?php endforeach; ?>
</table>

<ul>
Copy link
Contributor

Choose a reason for hiding this comment

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

Just remove the ul and li tags. They are unnecessary and if there is debate about the styling, just keep it as minimal as possible (a <br> could be enough).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed. They are really unnecessary.

Choose a reason for hiding this comment

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

indeed, ul and li should be remove. However, br is not a good id (should not be use outside of <p>) I think a simple div would be enough
I also agree about a separate file for delete form.
And about CSS, it should not be include (or with an option for included bootstrap classes maybe? not by default)

@codedmonkey
Copy link
Contributor

@sblondeau I immediately think of Bootstrap templates as well, but that's something that should be outside this bundle as these differences in styling are trivial at best when it's so easy to create your own maker. In fact, I'm planning on doing that exact thing if @sadikoff doesn't mind if I run off with some of his code.

@featuriz
Copy link

featuriz commented Feb 1, 2018

Thanks for making CRUD friends. I'm using symfony4.0.4 . I created maker extension as https://symfony.com/doc/current/bundles/SymfonyMakerBundle/index.html#creating-your-own-makers . It works great. The problems i faced are

  1. EntityManagerInterface::class, 'orm-pack' ;; @ MakeCrud Controller -> configureDependencies
    Error:
      [ERROR] Missing package: to use the make:crud command, run:                    
                                                                            
     composer require orm-pack

Allread orm-pack is installed, eventhough the error occurs.
Solution: I comment this check.

  1. ControllerWithTwig.tpl.php after generate, the controller throws error for new().
    Solution: I changed new() to newAction(). I added Action at the end of all the public (crud) methods.

  2. Template index.twig.html ;; Error for datetime entity.
    Solution: I manually added |date('m d y');

How to add date at index.tpl.php ;;

@sadikoff
Copy link
Contributor Author

sadikoff commented Feb 1, 2018

@featuriz

  1. was fixed in last code revision
  2. which php version do you use? from php 7.0 we can use reserved words as function names
  3. will think what I can do with this, I think the solution is to check fields and generate specific code for some types

@weaverryan
Copy link
Member

Thanks for getting this rolling! It’s on my radar - I’ll review this after I finish the make:entity changes :)

@featuriz
Copy link

featuriz commented Feb 2, 2018

Good Every thing works.
My suggestion is that.

  1. We are just creating the frame. So at create let it be " |date() ", timezone is not required, let them add it. Any how we have to update template as our taste.
  2. return must need " 'delete_form' => $deleteForm->createView(), " this, only then the generated twig will be complete. I think this is must for a good template.
  3. At show() route add " , requirements={"id"="\d+"} ". This will prevent parameter exception. i.e. /{id} and /new are same. Conflict!.
  4. Correct HTTP methods. https://stackoverflow.com/a/19728045/624533
  5. Need confirmation at delete. " < input type="submit" value="Delete" onclick="return confirm('Are you sure you want to delete this item?');"> "
  6. At ControllerWithTwig.tpl.php in delete method the token must be " _token"

This is for #113 (comment)

Also "Action()" needs to be added in all crud methods at ControllerWithTwig.tpl.php . Because I'm using XAMPP in which php is 7.1.11 . And the generated code throws syntax error for new() in NETBEANS 8.2. But php built in server runs without error.
from php 7.0 we can use reserved words as function names. :: OK, THANKS FOR YOUR INFO

These are the things I updated in my work.

@sadikoff
Copy link
Contributor Author

sadikoff commented Feb 5, 2018

@featuriz
1, 6 fixed.

3 ... requirements={"id"="\d+"} ...

it's not very good idea, in my project, for example, primary key is uuid and this requirement won't work. I fixed this problem with simple moving new action before show.

5 ... Need confirmation at delete

agreed but not on click, I think better will be to place it in onSubmit event

Need more discussion about 2 and 4. May be there are more ideas?


$input->setArgument('name', $value);

if ($this->entityHelper->isDoctrineInstalled()) {
Copy link
Member

Choose a reason for hiding this comment

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

We need to ask the below question in ALL cases, because the user may not use Doctrine, but may want to bind their form to a model class. In that case, of course, the $this->entityHelper->getEntitiesForAutocomplete() should return an empty array.

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.

Some comments - I think some last comments :)

I believe I still have a few previous comments that are todos for make:form, but I think you know that

Thanks!


$this->writeSuccessMessage($io);

$io->text('Next: Check your new crud!');
Copy link
Member

Choose a reason for hiding this comment

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

How about:

Next: Check your new CRUD by going to /sweet/food/

<div class="example-wrapper">
<h1>Edit <?= $entity_class_name ?></h1>

{{ include('<?= $route_name ?>/_form.html.twig', {'form': form, 'button_label': 'Edit'}) }}
Copy link
Member

Choose a reason for hiding this comment

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

'button_label': 'Update'

And no reason to have 'form': form - that variable will already be passed with that name

{% endblock %}

{% block body %}
<div class="example-wrapper">
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should add this class, or the CSS above it. I do like the idea of the CRUD not looking horrible initially... but the user WILL need to customize this and make it their own. So, we should not add any extra stuff that they will need to remove (or, we should be VERY minimalistic).

Actually, I talked to Javier, and we both agree that we should add a few basic Bootstrap classes e.g .<table class="table" and <button class="btn">. But, I think that's all we should include - we shouldn't package our own CSS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok good. It was just my experiment :) will add simple bootstrap classes


{{ include('<?= $route_name ?>/_form.html.twig', {'form': form, 'button_label': 'Edit'}) }}

<div class="example-actions">
Copy link
Member

Choose a reason for hiding this comment

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

And same here - you can remove this

<div class="example-actions">
<a href="{{ path('<?= $route_name ?>_index') }}">back to list</a>

{{ include('<?= $route_name ?>/_delete_form.html.twig', {'<?= $entity_var_singular ?>': <?= $entity_var_singular ?>}) }}
Copy link
Member

Choose a reason for hiding this comment

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

No need to pass the variable in explicitly - just {{ include(....) }} and that's it - no second argument :)

@@ -0,0 +1,6 @@
{{ form_start(form) }}
{{ form_widget(form) }}
<div>
Copy link
Member

Choose a reason for hiding this comment

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

Remove the div?

<form method="post" action="{{ path('<?= $route_name ?>_delete', {'<?= $entity_identifier ?>': <?= $entity_var_singular ?>.<?= $entity_identifier ?>}) }}" onsubmit="return confirm('Are you sure you want to delete this item?');">
<input type="hidden" name="_method" value="DELETE">
<input type="hidden" name="_token" value="{{ csrf_token('delete' ~ <?= $entity_var_singular ?>.<?= $entity_identifier ?>) }}">
<input type="submit" value="Delete">
Copy link
Member

Choose a reason for hiding this comment

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

Could we use a <button here?

class <?= $class_name ?> extends Controller
{
/**
* @Route("/", name="<?= $route_name ?>_index", methods="GET"): Response
Copy link
Member

Choose a reason for hiding this comment

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

The : Response shouldn't be here


public function __construct(FileManager $fileManager)
{
$this->baseLayoutExists = $fileManager->fileExists('templates/base.html.twig');
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 something you should calculate not in __construct() but only when you need it (so, down in getHeadPrintCode()). It doesn't really make a difference, but it's a better practice.

$printCode = $printCode.' ? '.$printCode.'|join(\', \') : \'\'';
} elseif (in_array($field['type'], ['boolean'])) {
$printCode = $printCode.' ? \'Yes\' : \'No\'';
}
Copy link
Member

Choose a reason for hiding this comment

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

We could clean this up by changing this to a switch ($field['type'])

/**
* @author Sadicov Vladimir <sadikoff@gmail.com>
*
* @internal
Copy link
Member

Choose a reason for hiding this comment

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

This one is actually not internal, as we're exposing it inside of the templates as a variable

*
* @internal
*/
class GeneratorTwigHelper
Copy link
Member

Choose a reason for hiding this comment

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

Add final class...

@@ -41,6 +51,25 @@ public function configureCommand(Command $command, InputConfiguration $inputConf
->addArgument('name', InputArgument::OPTIONAL, sprintf('The name of the form class (e.g. <fg=yellow>%sType</>)', Str::asClassName(Str::getRandomTerm())))
->setHelp(file_get_contents(__DIR__.'/../Resources/help/MakeForm.txt'))
;

$inputConf->setArgumentAsNonInteractive('name');
Copy link
Member

Choose a reason for hiding this comment

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

Well actually, you might still need to ask the bound-class value manually in interact so we can do some auto-completion :)

$entityClassDetails->getRelativeName(),
'Repository\\',
'Repository'
);
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 this is not quite right. The user could have setup their repository class to be any class - it may not follow the pattern we're expecting (and that's ok!). We need to read the actual customRepositoryClass from the entity metadata and use that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

really I didn't think about it. Will improve it. And I have one little idea about Form.

<?= $helper->getHeadPrintCode('New '.$entity_class_name) ?>

{% block stylesheets %}
<style>

Choose a reason for hiding this comment

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

style should not be here
I think it would be better to remove it and classes example-*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes =) just experiment I'll remove this stuff completely :)

@ihosav
Copy link

ihosav commented Mar 3, 2018

@sadikoff how could i use maker:crud now in symfony4?

@sadikoff
Copy link
Contributor Author

sadikoff commented Mar 3, 2018

@fictiont while this PR is not ready you can try to use my koff/crud-maker-bundle

Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

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

@sadikoff thanks for implementing this feature! Now that it's almost finished, I played with it and it's working as expected. I have some comments about very minor details:

  • We should use CRUD instead of crud everywhere except in the command name (make:crud) because CRUD is an acronym.
  • new, show, edit templates include a blank line before {% endblock %} which we could remove to be consistent with the other templates.
  • Sadly the Twig variables passed from controllers don't follow the official recommendations (e.g. ['sweetFoods' => $sweetFoods] should be ['sweet_foods' => $sweetFoods]) So we'd need to introduce new variables to replace this: ['<?= $entity_var_plural ?>' => $<?= $entity_var_plural ?>] by this: ['<?= $entity_plural_as_twig_var ?>' => $<?= $entity_plural_as_php_var ?>] (Not sure about the new var names)
  • I wonder if we should add some minimal CSS classes to actions like <a href="{{ path('sweet_food_index') }}">back to list</a> and <a href="{{ path('sweet_food_edit', {'id': sweetFood.id}) }}">edit</a> We already do that in Delete form (<button class="btn">Delete</button>). The reason is that "Back to list" and "Edit" are probably going to be styled differently by users, so a simple CSS could help them a lot.

And thanks for your patience during the review process. 112 comments and counting!

@sadikoff
Copy link
Contributor Author

sadikoff commented Mar 7, 2018

@javiereguiluz thanks for your review, it's really important for me!
I would like to discuss your last point about minimal CSS. Is it will be enough to style links as buttons with class="btn"?

@javiereguiluz
Copy link
Member

Not sure what to do, honestly. I'd say adding some minimal CSS classes (compatible with Bootstrap) would be nice for most people ... but maybe my guess is wrong and we make most people angry. @weaverryan what do you think about this? Thanks!

@jlcd
Copy link

jlcd commented Mar 8, 2018

Hey there @sadikoff , first of all thanks for the great work you've put in.

I've just tried to make a crud here and noticed that for my Location Entity it didn't quite work as expected.
The Entity in question has only one field, location, being also the Entity's ID.

When I ran make:crud Location, the controller and forms were created, but the new and edit forms are broken as they don't have the only table's field. Maybe it has something to do with the field being the ID field, not sure though.

Here's the Entity:

<?php

namespace App\Entity;

use Doctrine\ORM\Mapping as ORM;

/**
 * @ORM\Entity(repositoryClass="App\Repository\LocationRepository")
 */
class Location
{
    /**
     * @ORM\Id
     * @ORM\GeneratedValue(strategy="NONE")
     * @ORM\Column(type="string", length=60)
     */
    private $location;

    /**
     * @return string
     */
    public function getLocation()
    {
        return $this->location;
    }

    /**
     * @param string $location
     *
     * @return self
     */
    public function setLocation($location)
    {
        $this->location = $location;

        return $this;
    }
}

@sadikoff
Copy link
Contributor Author

sadikoff commented Mar 9, 2018

Hello @jlcd I've just tested your entity with last commited version here and everything works perfect. May be you are using my koff\crud-maker-bundle? Possibly there is a bug with some type of entities.

'Entity\\'
);

$entityDoctrineDetails = $this->entityHelper->createDoctrineDetails($entityClassDetails->getFullName());
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 we're missing some validation here to make sure that the class the user passed IS indeed a valid entity.

/**
* @author Sadicov Vladimir <sadikoff@gmail.com>
*/
final class DoctrineEntityDetails
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 this class can also be internal - i don't think you're passing it directly into the templates (which is good)

@sadikoff sadikoff changed the title [WIP] Add make:crud & improve make:form commands Add make:crud & improve make:form commands Mar 12, 2018
@sadikoff
Copy link
Contributor Author

Everything ready!

Thanks to all for reviews and discussion!

@weaverryan weaverryan merged commit 2683114 into symfony:master Mar 14, 2018
weaverryan added a commit that referenced this pull request Mar 14, 2018
…averryan)

This PR was merged into the 1.0-dev branch.

Discussion
----------

Add make:crud & improve make:form commands

Hi all,

## Improved make:form command
Main improvement is that command works in two ways. First way simple, it just generates skeleton form class and user can do with it whatever he want. Second way is generation from entity class. Generator checks if entity exists and ask user if he wants to generate form from entity or user can use `--entity` flag if he knows that entity exists.
### Generation features:
- Simple skeleton form class
- Doctrine entity form class
### Examples:
**Simple class generation**
```bash
root@striX: ~/project_root# bin/console make:form

 The name of the form class (e.g. GentleElephantType):
 > SimpleForm

Enter the class or entity name that the new form will be bound to (empty for none):
 >

 created: src/Form/SimpleFormType.php

  Success!
```
**Entity form generation**
```bash
root@striX: ~/project_root# bin/console make:form

 The name of the form class (e.g. GentleElephantType):
 > EntityFormType

Enter the class or entity name that the new form will be bound to (empty for none):
 > Entity

 created: src/Form/EntityFormType.php

  Success!
```
Closes: #114

## New make:crud command
Want to introduce `make:crud` command it is similar to doctrine:generate:crud from SensioGeneratorBundle.

All you need to generate cool crud is Entity :)
### Features
- complete crud generation
### Example:
```bash
root@striX: ~/project_root# bin/console make:crud SweetFood

 created: src/Controller/SweetFoodController.php
 created: src/Form/SweetFoodType.php
 created: templates/sweet_food/_delete_form.html.twig
 created: templates/sweet_food/_form.html.twig
 created: templates/sweet_food/index.html.twig
 created: templates/sweet_food/show.html.twig
 created: templates/sweet_food/new.html.twig
 created: templates/sweet_food/edit.html.twig

  Success!

```
Closes: #3

Commits
-------

2683114 some improvements
2ec8ff0 fix form variables
0d9e637 added validation on arguments
ada7db0 fix phpcs
f740e2e fix variables to follow official recommendations
b3bf878 some make:form improvements + experiments
204f5aa final fix for skeleton templates
96d54a7 Merge branch 'master' of https://github.com/sadikoff/maker-bundle
38b2864 Merge remote-tracking branch 'upstream/master'
ee98865 making test name less specific so we can run only it
ba0438f phpcs fix
3d319bd some skeleton improvements
da37780 Merge remote-tracking branch 'upstream/master'
2ebff8c fix cs and improvements according to review request
772f0db fix make:form
44b7851 completed improvement of make:form command
db30634 added tests for crud testig
ad8a4fb fix tests on appveyor
e8eabd6 Merge remote-tracking branch 'upstream/master'
60c0005 php cs fix
404c38f update all to lastest bundle code
cc4b758 Merge remote-tracking branch 'upstream/master'
1b95509 add csrf dependency it's necessary for delete action in crud and for better security
52a20bb improve make:crud command to use make:form logic for form generation
e24b22c fix to use with php 7.0
7555a3a improved make:form command
4045695 removed unnecessary code
7ed6b8b improved tests
3787739 cs fix
71d2e90 some code refactoring + testing new GeneratorHelper to simplify skeleton templates
6f1c296 array style fix
96f2af2 Merge remote-tracking branch 'upstream/master'
ab0a0dc Merge remote-tracking branch 'upstream/master'
422d31b Merge branch 'master' of https://github.com/sadikoff/maker-bundle
ff9a08d some fixes
42047a6 fix Token parameter name in delete action
5157e0a fix interception of /new url by show action
7783313 fix fobpot.io cs
31d9e81 fix templates generation with date fields in entity
1b0f17a test remove some phpdoc comments to pass travis.ci
207d8be add exception if entity doesn't exists
478b457 fix fabpot.io cs 3
791241d fix fabpot.io cs 2
9961f43 fix fabpot.io cs
f189efd fix tests
8c98b45 trying tests 2
08d4bc5 trying tests
8e9bd67 fix skeleton according to community recommendations
e9f91ef add MakeCrud class to makers.xml
6cdda8f final fix fabpot.ci CS
4c0b639 fix fabpot.ci CS
bf9a5c1 make:crud command initial commit
@weaverryan
Copy link
Member

Merged! And already tagged: https://github.com/symfony/maker-bundle/releases/tag/v1.2.0

Thanks you VERY much @sadikoff for very hard work, handling a lot of feedback and doing it quickly! I hope we can keep improving MakerBundle together :).

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.