Allow to configure properties for each listing and form field

[javiereguiluz]

NOTE: this is a big Pull Request and moves/reorganizes most of the existing code.

The purpose of this PR

Allow to configure listings and form fields properties (label, type, help message, CSS class, etc.)
Please, read the updated documentation in README.md file to better understand the new features.

The main changes of this PR

Initially, EasyAdmin had a huge controller which made everything. This PR introduces a proper Configurator service that does all the heavy configuration-related stuff and greatly reduces the code of the controller. In the future, we'll use APC cache or anything similar to cache in production all the entity configuration processing, which is the heaviest part of the backend and it can be done just once.

What I'd like to get reviewed

If someone is going to review this PR, it'd be very useful if he/she tries the new configuration options on a real backend and reports any problems. If you use "array" properties in your entities, you can also try the new cool JavaScript-based stuff to add elements dynamically in the forms.

In any case, please don't report missing PHPdoc blocks or very minor code styling issues because I'm going to polish all that before merging this PR. Thanks!

[Pierstoval]

With the high number of possibilities, I'm wondering if we'd have to make some phpunit tests in order to test all correct/incorrect behaviors...

[javiereguiluz]

@Pierstoval you are right. We need something ... but not yet. First, we'll iterate a bit over the configuration and once we settle on something stable, let's create the tests.

Related to this, in one of his great code reviews, @stof suggested that we could leverage the Config component features to do some sort of advanced configuration validation. He kindly offered himself to prepare something around this, so maybe we can take his word in the future and ask him to help us do something great with the Config component.

[Pierstoval]

+1 for the Config component, I saw that you just use the "VariableNode" , which is kinda "light" for this kind of behavior.

Plus, I'd recommend to make tests the sooner we can, because this bundle will only grow up in time, and the bigger it is, the harder the test will be to develop... 😲

Creating a "Config tester" is kinda hard, I looked at Fos, Sonata Project, Symfony and Doctrine repositories to see examples of configuration tests, many of them focus on the service definition, some of them look at the configuration, but they almost all have a different method... Putting confs in yaml and xml fixtures, putting them raw in the Test class, etc.

I made a little "test tester" on my local repo, I'll push it during the afternoon you can see it here, maybe @stof can be available to check if it's a good one to start with or not :)

[xabbuh]

Creating a "Config tester" is kinda hard, I looked at Fos, Sonata Project, Symfony and Doctrine repositories to see examples of configuration tests, many of them focus on the service definition, some of them look at the configuration, but they almost all have a different method... Putting confs in yaml and xml fixtures, putting them raw in the Test class, etc.

You may want to take a look at the matthiasnoback/symfony-config-test package by @matthiasnoback.

[Pierstoval]

Thanks @xabbuh I'll check this out to make better config tests 👍

To come back to the subject, I've tested some config schemes, and I think that the fields array "prototype" is too restrictive.
The class attribute only applies to the created widget (input, select, textarea ...) , but if I want to add a custom class to the label, it's actually impossible.

IMO, we should find a solution to merge a formField attribute with usual form field definition we use in our FormTypes classes.
For example, with my TranslationBundle:

easy_admin:
    entities:
        Translation:
            class: Pierstoval\Bundle\TranslationBundle\Entity\Translation
            form:
                fields:
                    - token
                    - source
                    - translation
                    - { property: locale, label: "Langue", type: "choice", "help": "Choisissez une langue", class: "test" }
                    - domain

The possible choices should be the %locales% parameter, parsed by the PierstovalTranslationExtension class. Actually, there is no way of defining it in EasyAdmin, so to handle the new and edit functions for the Translation Entity, I may need to create my own controller.

It would be great if we had a formField attribute (or any possible name, like formType or whatever) to pass parameters like the ones I'd put in my FormType:

$formBuilder->add('locale', 'choice', array('choices' => $locales));

This is my first issue, I may have some other in the future :)

[Pierstoval]

I've got a really minor suggestion : You may need to rebase this branch with your master, at least to integrate #32 ;)

[javiereguiluz]

If someone else wants to review if this PR is working right before merging it, you only have to execute two commands to download this PR as a new branch:

# you may need to change 'origin' by 'upstream' depending on how you use Git
$ git fetch -fu origin refs/pull/64/head:pr/64

$ git checkout pr/64

[stof]

The namespace is weird. Service has no meaning as far as the class is concerned (a service is an instance of the class registered in the Symfony DIC, but the class is not aware of it)

[Pierstoval]

What would you recommend then ?
JavierEguiluz\Bundle\EasyAdminBundle\Configuration ?

[javiereguiluz]

@stof, yes Service is a bad name, but I can't think of a better name right now :(

@Pierstoval the problem with your proposal is that the FQCN looks a bit verbose to me: --\EasyAdminBundle\Configuration\Configurator.

Let's think about other alternatives.

[Pierstoval]

Because you think that Symfony\Component\Form\Form is not too verbose ? 😉

Maybe we can rename the configurator into EntityConfigurator , so the FQCN would be JavierEguiluz\Bundle\EasyAdminBundle\Configuration\EntityConfigurator , to allow one day to create a DocumentConfigurator for Doctrine ODM :)

[stof]

Configuration is indeed much better than Service, as it relates to the goal of the class rather than the way it is used. However, it indeed looks duplicate in the FQCN. I don't have good ideas about naming it though

[javiereguiluz]

@Pierstoval to be honest, everything about the Form component looks verbose and complicated to me, not only the FQCN.

I don't like the EntityGenerator class name because it's too long and this project will probably never add support for Doctrine ODM, Propel, etc. The reason is that there are already good admin generators that support all of that, such as AdmingeneratorGeneratorBundle

[javiereguiluz]

This PR adds support for adding collection items dynamically:

As this requires to execute some JavaScript code, it'd be great if you could test it to ensure that no JavaScript errors happen in your browser.

[Pierstoval]

Awesome !! Didn't notice this one ! 👍

[javiereguiluz]

@Pierstoval regarding your issue about advanced customization of form fields, I'm not sure it's something we can do in config files. I'm updated the documentation in 7a41681 to show how to do what you asked for in the example of the choice field with custom values.

[Pierstoval]

I looked at it, and it's GREAT, it really looks like Sonata's Admin class system, but inside a controller, so we have access to all the application & container, and can customize routes, which is really better 👍

[Pierstoval]

When this PR is merged and correctly set up, I may work on the Configuration class to allow this kind of validation directly from the builder.
I prefer to wait before it's fully merged because you may make some modifications, and I want the feature to be fully designed before working on the future ;)

As @MacFJA said in #43 , use the field name as key could improve really better the understandability of the configuration, so ... ;)

[MacFJA]

Why don't use the DoctrineOrmTypeGuesser ( Symfony\Bridge\Doctrine\Form\DoctrineOrmTypeGuesser::guessType ) instead of hard-coding a mapping?

[javiereguiluz]

In other cases, I would agree to reuse the existing code instead of reinventing it. However, there are three main reasons why I don't like this DoctrineOrmTypeGuesser class:

1. It calls the method to get the class metadata. If you don't use metadata cache, this introduces yet another performance penalty for no reason.
2. I find it lacking for some types. For instance, json_array and simple_array types would be displayed with text form field instead of the more natural textarea and collection.
3. It looks more complicated than it should be, with those "guess probabilities".

Moreover, there are two arguments in favor of using our own custom (and simple) solution:

1. It requires zero maintenance because new Doctrine types are very rarely added.
2. It will allow in the future to use our custom form fields. In my opinion, Form component is very lacking of form field types for real-world applications. In the future, I'd like EasyAdmin to provide what people expect from a Form component: dynamic datetime selects, color pickers, WYSIWYG textareas, dependent select lists, multiple files uploader, etc.

[stof]

@javiereguiluz the metadata are cached in memory for the current request in all cases. Given that Doctrine already loaded the metadata (as you loaded an object), the first point is void.

but anyway, I agree with your points about not using it