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

Implement dynamic form customization #8368

Merged
merged 4 commits into from Mar 30, 2018

Conversation

Projects
None yet
9 participants
@mickaelandrieu
Contributor

mickaelandrieu commented Sep 26, 2017

Questions Answers
Branch? develop
Description? You want to add your own form fields inside new Symfony pages and you rant against 1.7 cause you can't? Stop rant and start to give us some love now :)
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? http://forge.prestashop.com/browse/BOGOSS-48
How to test? See how tests are done, and use this module.
.

form

TodoList

  • Add form input into every new forms
  • Validate this new fields
  • Persist and retrieve the data
  • Introduce new Errors management system (out of scope)
  • Add hooks / "form_rest" calls for every migrated pages:
    • Administration Page
    • System Information Page (no form)
    • Module Catalogue Page (no form)
    • Module Management Page (do we want modules to alter our store? o_O)
    • Performance Page
    • Maintenance Page
    • Product Page if this PR is merged (can be done later)
  • Docs on Prestafony to explain additional steps on page migration.

Docs

Create the module and register the hooks

<?php
# /modules/module_name/module_name.php

    public function hookDisplayAdministrationPageForm(&$hookParams)
    {
        $formBuilder = $hookParams['form_builder'];
        $uploadQuotaForm = $formBuilder->get('upload_quota');
        $uploadQuotaForm->add('description', TextType::class, [
            'data' => 'A description',
            'label' => 'Description'
        ]);
    }

    public function hookActionAdministrationPageFormSave(&$hookParams)
    {
        // retrieve and validate the data
        dump($hookParams['form_data']['upload_quota']['description']);

        // if the data is invalid, populate `errors` array
        dump($hookParams['errors']);
    }

Expected result: the form field should be available in the selected form, can be validated and persisted in the database if valid using the provided hooks.

Templating

Of course, you can override every template to improve again the rendering of the form (the Back Office theme may be/will be improved in future versions)

# /modules/module_name/views/PrestaShop/Admin/AdvancedParameters/administration.html.twig
{% block administration_form_upload_quota %}
            <div class="col">
                <div class="card">
                    <h3 class="card-header">
                        <i class="material-icons">file_upload</i> {{ 'Upload quota'|trans }}
                    </h3>
                    <div class="card-block">
                        <div class="card-text">
                            <div class="form-group">
                                {{ ps.label_with_help(('Maximum size for attached files'|trans), ('Set the maximum size allowed for attachment files (in megabytes). This value has to be lower or equal to the maximum file upload allotted by your server (currently: %size% MB).'|trans({'%size%': 'PS_ATTACHMENT_MAXIMUM_SIZE'|configuration}, 'Admin.Advparameters.Help'))) }}
                                {{ form_errors(uploadQuotaForm.max_size_attached_files) }}
                                {{ form_widget(uploadQuotaForm.max_size_attached_files) }}
                            </div>
                            <div class="form-group">
                                {{ ps.label_with_help(('Maximum size for a downloadable product'|trans), ('Define the upload limit for a downloadable product (in megabytes). This value has to be lower or equal to the maximum file upload allotted by your server (currently: %size% MB).'|trans({'%size%': 'PS_LIMIT_UPLOAD_FILE_VALUE'|configuration}, 'Admin.Advparameters.Help'))) }}
                                {{ form_errors(uploadQuotaForm.max_size_downloadable_product) }}
                                {{ form_widget(uploadQuotaForm.max_size_downloadable_product) }}
                            </div>
                            <div class="form-group">
                                {{ ps.label_with_help(("Maximum size for a product's image"|trans), ('Define the upload limit for an image (in megabytes). This value has to be lower or equal to the maximum file upload allotted by your server (currently: %size% MB).'|trans({'%size%': 'PS_LIMIT_UPLOAD_IMAGE_VALUE'|configuration}, 'Admin.Advparameters.Help'))) }}
                                {{ form_errors(uploadQuotaForm.max_size_product_image) }}
                                {{ form_widget(uploadQuotaForm.max_size_product_image) }}
                            </div>

                           {# Do what you need to do, I'm really bad at designing pages ^o^ #}
                            <div class="form-group">
                               {{ form_label(uploadQuotaForm.description) }}
                               {{ form_widget(uploadQuotaForm.description) }}
                               {{ form_errors(uploadQuotaForm.description) }}
                            </div>
                            {{ form_rest(uploadQuotaForm) }}
                        </div>
                    </div>
                    <div class="card-footer">
                        <div class="d-flex justify-content-end">
                            <button class="btn btn-primary">{{ 'Save'|trans({}, 'Admin.Actions') }}</button>
                        </div>
                    </div>
                </div>
            </div>
        {% endblock %}

This change is Reviewable

@PrestaShop PrestaShop deleted a comment from prestonBot Sep 26, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 26, 2017

@mickaelandrieu mickaelandrieu changed the title from Implement dynamic form customization to [WIP] Implement dynamic form customization Sep 27, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 27, 2017

@codacy-bot

This comment has been minimized.

Show comment
Hide comment
@codacy-bot

codacy-bot Sep 27, 2017

Codacy Here is an overview of what got changed by this pull request:

Issues
======
- Added 1
           

Complexity increasing per file
==============================
- src/PrestaShopBundle/Form/Admin/Type/CommonAbstractType.php  2
         

See the complete overview on Codacy

codacy-bot commented Sep 27, 2017

Codacy Here is an overview of what got changed by this pull request:

Issues
======
- Added 1
           

Complexity increasing per file
==============================
- src/PrestaShopBundle/Form/Admin/Type/CommonAbstractType.php  2
         

See the complete overview on Codacy

@mickaelandrieu mickaelandrieu added this to the 1.7.4.0 milestone Nov 8, 2017

@LittleBigDev LittleBigDev added the WIP label Nov 22, 2017

@mickaelandrieu mickaelandrieu changed the title from [WIP] Implement dynamic form customization to Implement dynamic form customization Nov 28, 2017

@oleacorner

This comment has been minimized.

Show comment
Hide comment
@oleacorner

oleacorner Dec 1, 2017

Contributor

Hi Mickael,

I don't know if my request could be part of this pull request
For some of my module I sometimes need to be able to have several categories selectors in one form. Up to now, I have to override the category form field to be able to do this.

Contributor

oleacorner commented Dec 1, 2017

Hi Mickael,

I don't know if my request could be part of this pull request
For some of my module I sometimes need to be able to have several categories selectors in one form. Up to now, I have to override the category form field to be able to do this.

@kpodemski

This comment has been minimized.

Show comment
Hide comment
@kpodemski

kpodemski Dec 1, 2017

Contributor

@oleacorner

it's required to modify category_tree_helper and rely on element ID, remember that you can easily override helpers from your module by placing tpl in views/templates/admin/helpers/ etc.

Contributor

kpodemski commented Dec 1, 2017

@oleacorner

it's required to modify category_tree_helper and rely on element ID, remember that you can easily override helpers from your module by placing tpl in views/templates/admin/helpers/ etc.

@oleacorner

This comment has been minimized.

Show comment
Hide comment
@oleacorner

oleacorner Dec 1, 2017

Contributor

@kpodemski
Sure, I already override the category_tree_helper from my module.
But the helper should not embed the ID, it should be given as parameter. the helper could then be reusable without overrive

Contributor

oleacorner commented Dec 1, 2017

@kpodemski
Sure, I already override the category_tree_helper from my module.
But the helper should not embed the ID, it should be given as parameter. the helper could then be reusable without overrive

@kpodemski

This comment has been minimized.

Show comment
Hide comment
@kpodemski

kpodemski Dec 1, 2017

Contributor

@oleacorner by writing "element ID" i meant that we should be able to provide id in array of the form element, you're right, but this commit has nothing to do with that :P you can create forge ticket with improvement request

Contributor

kpodemski commented Dec 1, 2017

@oleacorner by writing "element ID" i meant that we should be able to provide id in array of the form element, you're right, but this commit has nothing to do with that :P you can create forge ticket with improvement request

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Mar 20, 2018

Contributor

ping @kpodemski @PrestaEdit: wdyt? :) Will works on every form on pages migrated to Symfony.

Available from 1.7.4.

Contributor

mickaelandrieu commented Mar 20, 2018

ping @kpodemski @PrestaEdit: wdyt? :) Will works on every form on pages migrated to Symfony.

Available from 1.7.4.

@kpodemski

This comment has been minimized.

Show comment
Hide comment
@kpodemski

kpodemski Mar 20, 2018

Contributor

hey @mickaelandrieu

you know that it's crucial for bigger projects to have it, i'm happy that you're putting a time to work on this

from what i see at first is that every Symfony form have to invoke form_rest(nameOfTheForm), right? so the forms are "extended", not "customized", which means that we cannot remove, add fields in between form?

edit: ok, for the form modification, i guess we can just override template, additional forms are coming to the end of the form, am i right?

btw. i haven't checked it yet, do we have an option to add our own fully Symfonized (:D) tab?

Contributor

kpodemski commented Mar 20, 2018

hey @mickaelandrieu

you know that it's crucial for bigger projects to have it, i'm happy that you're putting a time to work on this

from what i see at first is that every Symfony form have to invoke form_rest(nameOfTheForm), right? so the forms are "extended", not "customized", which means that we cannot remove, add fields in between form?

edit: ok, for the form modification, i guess we can just override template, additional forms are coming to the end of the form, am i right?

btw. i haven't checked it yet, do we have an option to add our own fully Symfonized (:D) tab?

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Mar 20, 2018

Contributor

from what i see at first is that every Symfony form have to invoke form_rest(nameOfTheForm), right? so the forms are "extended", not "customized", which means that we cannot remove, add fields in between form?

Extended and Customized! Basically, if you declare a form by his name that already exists (let's say "meta_description" of ProductSeo form, your declaration will replace the existing one. Others will be displayed without no intervention thanks to the form_rest helper (this is why I need to add it to every template). Sadly, we can't re-order form inputs in hooks... for now (in 1.7.5 maybe ;) ).

ok, for the form modification, i guess we can just override template, additional forms are coming to the end of the form, am i right?

You're right! You can override templates and place each field everywhere, in the order and with the specific design/js behavior your customers want. By default, forms are just added in the order they were added to the bottom of the form (in place of form_rest helper). So in the hooks, order matters.

btw. i haven't checked it yet, do we have an option to add our own fully Symfonized (:D) tab?

The tab management is not part of this pull request, I don't think I'll have time to provide this feature if it's missing (maybe it exists but as almost all is undocumented... :'( ), because we are close to the freeze of 1.7.4.x release and I need to finish this pull request, add tests and document it.

which means that we cannot remove

One more thing :p, it's totally valid to remove a field with the form builder:

$formBuilder->remove('field_name');

The real issue is that you can't (and won't be able to) change the validation of native forms. Let's take the example of Administration page. If you try to remove the field with the label "Maximum size for attached files". You can remove it thanks to the formBuilder but the validation of form will fail. This is intended "by design" because we want to cover our core features with tests. Maybe, in this case, you can transform your field into a hidden input or add an empty value in process action, I don't know.

Contributor

mickaelandrieu commented Mar 20, 2018

from what i see at first is that every Symfony form have to invoke form_rest(nameOfTheForm), right? so the forms are "extended", not "customized", which means that we cannot remove, add fields in between form?

Extended and Customized! Basically, if you declare a form by his name that already exists (let's say "meta_description" of ProductSeo form, your declaration will replace the existing one. Others will be displayed without no intervention thanks to the form_rest helper (this is why I need to add it to every template). Sadly, we can't re-order form inputs in hooks... for now (in 1.7.5 maybe ;) ).

ok, for the form modification, i guess we can just override template, additional forms are coming to the end of the form, am i right?

You're right! You can override templates and place each field everywhere, in the order and with the specific design/js behavior your customers want. By default, forms are just added in the order they were added to the bottom of the form (in place of form_rest helper). So in the hooks, order matters.

btw. i haven't checked it yet, do we have an option to add our own fully Symfonized (:D) tab?

The tab management is not part of this pull request, I don't think I'll have time to provide this feature if it's missing (maybe it exists but as almost all is undocumented... :'( ), because we are close to the freeze of 1.7.4.x release and I need to finish this pull request, add tests and document it.

which means that we cannot remove

One more thing :p, it's totally valid to remove a field with the form builder:

$formBuilder->remove('field_name');

The real issue is that you can't (and won't be able to) change the validation of native forms. Let's take the example of Administration page. If you try to remove the field with the label "Maximum size for attached files". You can remove it thanks to the formBuilder but the validation of form will fail. This is intended "by design" because we want to cover our core features with tests. Maybe, in this case, you can transform your field into a hidden input or add an empty value in process action, I don't know.

@alegout

This comment has been minimized.

Show comment
Hide comment
@alegout

alegout Mar 28, 2018

Contributor

Reviewed 1 of 10 files at r2, 1 of 13 files at r3, 1 of 4 files at r4, 26 of 37 files at r5, 1 of 1 files at r6, 19 of 19 files at r7, 6 of 6 files at r8.
Review status: 32 of 34 files reviewed at latest revision, 7 unresolved discussions.


app/AppKernel.php, line 78 at r8 (raw file):

        if ($this->parametersFileExists()) {
            try {
                $this->enableComposerAutoloaderOnModules($this->getActiveModules());

Do we really need this? Maybe has an impact on perf on big site?


src/Adapter/LegacyHookSubscriber.php, line 290 at r7 (raw file):

Previously, mickaelandrieu (Mickaël Andrieu) wrote…

it's not the same issue, I have added "0" as a specific use case to allow the Hook Collector to collect hooks that are not collected if no modules use it.

#8463 solves the "undefined module id" when my update introduces the "module with id 0" to allow the DataCollector to get all hooks.

Real problem behind no?


src/PrestaShopBundle/Form/Admin/AdvancedParameters/Performance/PerformanceFormHandler.php, line 83 at r8 (raw file):

        ;

        $this->hookDispatcher->dispatchForParameters('displayPerformancePageForm', ['form_builder' => &$formBuilder]);

Looks like these calls and those in save may be mutualized in parent and pass a param for the hook name? What do you think? They look the same in many form files.


src/PrestaShopBundle/Resources/config/services/services.yml, line 314 at r8 (raw file):

        class: PrestaShop\PrestaShop\Core\Form\AbstractFormProvider
        calls:
            - [setHookDispatcher, ['@prestashop.hook.dispatcher']]

is needed? cause you also do it in compilerpass?


.travis.yml, line 4 at r4 (raw file):

addons:
    #chrome: stable

is this normal?


Comments from Reviewable

Contributor

alegout commented Mar 28, 2018

Reviewed 1 of 10 files at r2, 1 of 13 files at r3, 1 of 4 files at r4, 26 of 37 files at r5, 1 of 1 files at r6, 19 of 19 files at r7, 6 of 6 files at r8.
Review status: 32 of 34 files reviewed at latest revision, 7 unresolved discussions.


app/AppKernel.php, line 78 at r8 (raw file):

        if ($this->parametersFileExists()) {
            try {
                $this->enableComposerAutoloaderOnModules($this->getActiveModules());

Do we really need this? Maybe has an impact on perf on big site?


src/Adapter/LegacyHookSubscriber.php, line 290 at r7 (raw file):

Previously, mickaelandrieu (Mickaël Andrieu) wrote…

it's not the same issue, I have added "0" as a specific use case to allow the Hook Collector to collect hooks that are not collected if no modules use it.

#8463 solves the "undefined module id" when my update introduces the "module with id 0" to allow the DataCollector to get all hooks.

Real problem behind no?


src/PrestaShopBundle/Form/Admin/AdvancedParameters/Performance/PerformanceFormHandler.php, line 83 at r8 (raw file):

        ;

        $this->hookDispatcher->dispatchForParameters('displayPerformancePageForm', ['form_builder' => &$formBuilder]);

Looks like these calls and those in save may be mutualized in parent and pass a param for the hook name? What do you think? They look the same in many form files.


src/PrestaShopBundle/Resources/config/services/services.yml, line 314 at r8 (raw file):

        class: PrestaShop\PrestaShop\Core\Form\AbstractFormProvider
        calls:
            - [setHookDispatcher, ['@prestashop.hook.dispatcher']]

is needed? cause you also do it in compilerpass?


.travis.yml, line 4 at r4 (raw file):

addons:
    #chrome: stable

is this normal?


Comments from Reviewable

@alegout

This comment has been minimized.

Show comment
Hide comment
@alegout

alegout Mar 28, 2018

Contributor

Reviewed 2 of 2 files at r9.
Review status: all files reviewed at latest revision, 10 unresolved discussions.


Comments from Reviewable

Contributor

alegout commented Mar 28, 2018

Reviewed 2 of 2 files at r9.
Review status: all files reviewed at latest revision, 10 unresolved discussions.


Comments from Reviewable

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Mar 28, 2018

Contributor

Do we really need this? Maybe has an impact on perf on big site?

Not anymore for this pr at least, I remove (out of scope)

Real problem behind no?

real problem is the legacyhooksubscriber itself who ignore hooks that are not registered by modules, thinking about it, I can remove it from this pr (out of scope)

Looks like these calls and those in save may be mutualized in parent and pass a param for the hook name? What do you think? They look the same in many form files.

I'm not sure I will pass the same hooks or same data everywhere, and I don't want to be stuck later: can we talk again about it when we will have migrated 10/12 more pages?

is needed? cause you also do it in compilerpass?

I ... don't have compiler pass? well, it's not needed 🤣

is this normal?

it's a reviewable bug ^^

Contributor

mickaelandrieu commented Mar 28, 2018

Do we really need this? Maybe has an impact on perf on big site?

Not anymore for this pr at least, I remove (out of scope)

Real problem behind no?

real problem is the legacyhooksubscriber itself who ignore hooks that are not registered by modules, thinking about it, I can remove it from this pr (out of scope)

Looks like these calls and those in save may be mutualized in parent and pass a param for the hook name? What do you think? They look the same in many form files.

I'm not sure I will pass the same hooks or same data everywhere, and I don't want to be stuck later: can we talk again about it when we will have migrated 10/12 more pages?

is needed? cause you also do it in compilerpass?

I ... don't have compiler pass? well, it's not needed 🤣

is this normal?

it's a reviewable bug ^^

@alegout

This comment has been minimized.

Show comment
Hide comment
@alegout

alegout Mar 28, 2018

Contributor

Looks like these calls and those in save may be mutualized in parent and pass a param for the hook name? What do you think? They look the same in many form files.

I'm not sure I will pass the same hooks or same data everywhere, and I don't want to be stuck later: can we talk again about it when we will have migrated 10/12 more pages?

Yes np, it was just an idea, not a blocking point.

Contributor

alegout commented Mar 28, 2018

Looks like these calls and those in save may be mutualized in parent and pass a param for the hook name? What do you think? They look the same in many form files.

I'm not sure I will pass the same hooks or same data everywhere, and I don't want to be stuck later: can we talk again about it when we will have migrated 10/12 more pages?

Yes np, it was just an idea, not a blocking point.

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Mar 28, 2018

Contributor

@alegout comments addressed, thanks for review 👍

Contributor

mickaelandrieu commented Mar 28, 2018

@alegout comments addressed, thanks for review 👍

@alegout

This comment has been minimized.

Show comment
Hide comment
@alegout

alegout Mar 28, 2018

Contributor

Reviewed 2 of 2 files at r10.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


src/Adapter/LegacyHookSubscriber.php, line 290 at r7 (raw file):

Previously, alegout (Alban) wrote…

Real problem behind no?

Ok so it's another problem with a different ticket.


Comments from Reviewable

Contributor

alegout commented Mar 28, 2018

Reviewed 2 of 2 files at r10.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


src/Adapter/LegacyHookSubscriber.php, line 290 at r7 (raw file):

Previously, alegout (Alban) wrote…

Real problem behind no?

Ok so it's another problem with a different ticket.


Comments from Reviewable

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Mar 28, 2018

Contributor

Ok so it's another problem with a different ticket

yes :)

Contributor

mickaelandrieu commented Mar 28, 2018

Ok so it's another problem with a different ticket

yes :)

@alegout

This comment has been minimized.

Show comment
Hide comment
@alegout

alegout Mar 28, 2018

Contributor

:lgtm:


Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

Contributor

alegout commented Mar 28, 2018

:lgtm:


Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@@ -75,17 +76,10 @@ public function updateConfiguration(array $configuration)
*/
public function validateConfiguration(array $configuration)
{
$resolver = new OptionsResolver();

This comment has been minimized.

@alegout

alegout Mar 28, 2018

Contributor

Why removing all OptionsResolver?

@alegout

alegout Mar 28, 2018

Contributor

Why removing all OptionsResolver?

Done

@marionf marionf added QA ✔️ and removed waiting for QA labels Mar 29, 2018

@alegout alegout merged commit 126a1e6 into PrestaShop:develop Mar 30, 2018

1 of 3 checks passed

Codacy/PR Quality Review Not so good... This pull request quality could be better.
Details
code-review/reviewable 7 files, 1 discussion left (alegout, Quetzacoalt91)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mickaelandrieu mickaelandrieu deleted the mickaelandrieu:bogoss/dynamic-forms branch Apr 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment