Prestashop: Make symfony forms easier to extend from modules using form_rest

[sarjon]

Modules can extend Symfony forms by adding new fields, but form templating is a bit tricky, see example below.

Let's say we want to customize Email configuration form. We have added new fields using hook, but form does not look pretty. So we want to customize template.

Take this https://github.com/PrestaShop/PrestaShop/blob/develop/src/PrestaShopBundle/Resources/views/Admin/Configure/AdvancedParameters/Email/index.html.twig#L42 as example.

In order to render our field we need to replicate exact path of template in our module modules/my_module/views/PrestaShop/Admin/Configure/AdvancedParameters/Email/index.html.twig and copy whole content of original template inside our module's template. Then we can add our custom field rendering and it works. However, if template were to change (styling, classes, layout & etc) in core, module would still be using outdated template (unless updated whole content manually).

I see 2 options how we could improve that:

1. Add empty template in each form so it can be overridden without copy whole contents. See example:

{# PrestaShopBundle/Resources/views/Admin/Configure/AdvancedParameters/Email/index.html.twig #}

<div class="content">
  {# Start form rendering here ....... #}
  {{ form_start(form) }}

  {# Include single template so module can override it without copy whole content of current template. #}
  {# Inside included template `form` variable would be available #}
  {% include 'PrestaShopBundle/Resources/views/Admin/Configure/AdvancedParameters/Email/Blocks/empty_form_customization.html.twig' %}

  {{ form_end(form) }}
  {# Ending form rendering here ....... #}
</div>

By doing this, multiple modules can add custom fields to forms but only 1 module can extend template and render it how it wants to.

1. Add hook inside template so multiple modules can render their custom fields inside template without even overriding core templates. See example below:

{# PrestaShopBundle/Resources/views/Admin/Configure/AdvancedParameters/Email/index.html.twig #}

<div class="content">
  {# Start form rendering here ....... #}
  {{ form_start(form) }}

  {# Allow multiple modules render their custom form fields. #}
  {{ hook('renderSomeForm', {'form': form}) }}

  {{ form_end(form) }}
  {# Ending form rendering here ....... #}
</div>

i know @mickaelandrieu was against 2nd option, but i dont see why? hooks are still present in PS and they wont go away at least until next major, right? so why not letting multiple modules extend symfony forms? :thinking:

so @mickaelandrieu @matks wdyt?

[kpodemski]

@sarjon

what about something like smarty extend and smarty blocks? how it is working in Twig?

so developer would create:
modules/my_module/views/PrestaShop/Admin/Configure/AdvancedParameters/Email/index.html.twig

with content only like:

{extend 'originalfile'}
{block name='additionalInputs' append}
something something
{/block}

i'm not familiar with Twig enough to tell you how this should be done but i'm assuming that there's some option in Twig to do that, right?

Aaand, if that's not a good idea, i would love to see 2nd option, hooks are not bad and replacing whole file just to add something is imho much worst idea

[sarjon]

@kpodemski it is possible to use blocks. However, it does not work because {% extends ... %} ends up in recursion. :/ I think this happens because of path resolving that it want to replace core template with module template infinitely.

[kpodemski]

oh :( it was very convenient way to do such a stuff using Smarty, I'll try to do some research, it's hard to believe that Twig is less flexible than Smarty :P

[sarjon]

@kpodemski its not because of Twig, it because how PrestaShop uses it to find templates. :)

[123monsite-regis]

i'm in favour of seconf solution, {{ hook('renderSomeForm', {'form': form}) }}
we use this way with such already presents hooks such as 'displayAdminProductsMainStepRightColumnBottom' , 'displayAdminProductsMainStepLeftColumnMiddle' etc,
and having on our modules like that

public function hookDisplayAdminProductsMainStepRightColumnBottom($params)
{
    [...]
    $form_factory = $this->get('form.factory');
    $options = array('csrf_protection' => false);
    //here binfing a standart Symfony form
    $form = $form_factory->createNamed('OURMODULE_PREFIX_FOR_INPUTS', 'OURMODULE\Form\Admin\Product\OURMODULEAdminForm', $current_data, $options);

    return $this->get('twig')->render('@PrestaShop/OURMODULE/AdminProduct/hooks-form.html.twig',[
            'form' => $form->createView(),
        ]);
    [...]
}

and the both templates below:
OURMODULE/AdminProduct/hooks-form.html.twig

{% form_theme form '@PrestaShop/OURMODULE/OURMODULE/form.html.twig' %}

{{ form_errors(form) }}
{{ form_widget(form) }}

OURMODULE/OURMODULE/form.html.twig

{% use "PrestaShopBundle:Admin/TwigTemplateForm:bootstrap_4_horizontal_layout.html.twig" %}
{% block form_label_class -%}
   my-from
{%- endblock form_label_class %}

the main advangage, is that we can reuse the same FormBuilder on the hook storing the datas

it works pretty well

[matks]

I like the hook solution too because it reuses a well known and powerful mechanism of PrestaShop, so we stay in common ground and helps developers travel from 1.6 to 1.7 and to Symfony using things they are familiar with.

Maybe Mickael was rather for solution 1 because I think it benefits from Twig cache optimization while solution 2 is specific to PrestaShop and less prone to templating performance boost.

[sarjon]

ping @matks as a reminder. ^^

[tomas862]

I have crazy but rather quite good and simple idea we can implement here.

We can choose 2 option which is mentioned here https://github.com/PrestaShop/PrestaShop/issues/12585#issue-410665476 but rather when enforcing everyone to add custom hooks inside every template we can hide implementation here: https://github.com/PrestaShop/PrestaShop/blob/develop/src/PrestaShopBundle/Resources/views/Admin/TwigTemplateForm/form_div_layout.html.twig#L343

in short: we add display hook call in form_rest rendering.

benefits:

• no need to remember to add display hook in every form.
• will work for any form that is being rendered.

since we need to user form_rest everywhere developers will only need to be reminded just to add form_rest inside the <div class="card-text"></div> so it will render nicely with the rest of the form :)

[matks]

@tomas862 this is sneaky 😄but indeed it solves the issue of making sure everybody puts the required hook.

Only thing I'm worried about is performance overheard. I'd like us to analyze it (maybe we need a POC) to make sure the additional processing there is not too expensive for template rendering. Because this hook is going to be called everywhere we use form_rest()

[rokaszygmantas]

@matks I've created a PR with the idea that @tomas862 suggested (hook in form_rest), it doesn't seem to affect performance and allows to render the form fields nicely from modules :slightly_smiling_face:

[matks]

After discussing with @eternoendless, here's the plan:

• hook in form_rest https://github.com/PrestaShop/PrestaShop/pull/13412 will be in the scope of 1.7.7
• however we can consider the bad display (see below) for added fields is a 1.7.6 bug so we can improve the default form theme https://github.com/PrestaShop/PrestaShop/pull/11386 to make it right and merge this in 1.7.6 scope

Bad display, without rendering: https://prnt.sc/nc96pk

[matks]

oh :( it was very convenient way to do such a stuff using Smarty, I'll try to do some research, it's hard to believe that Twig is less flexible than Smarty :P

Good news: we were wrong, it works ! 🎉 See https://github.com/PrestaShop/docs/pull/274

[matks]

Form theme has been updated, form_rest will hooking capabilities will be implemented in 1.7.7 following https://github.com/PrestaShop/PrestaShop/issues/12585#issuecomment-491285862

[matks]

Since https://github.com/PrestaShop/PrestaShop/pull/15130/ has been merged (thanks @sarjon !) we have the ability to render **whole form using a single form_rest call in Twig).

Now we need to apply this everywhere 😄

[matks]

Follow up will be done https://github.com/PrestaShop/PrestaShop/issues/16482

I close this now.