[POC] Add a hook to form_rest for custom field rendering #13412

rokaszygmantas · 2019-04-15T15:32:12Z

Questions	Answers
Branch?	develop
Description?	Adds a hook that allows to customize form fields rendering from a module
Type?	new feature
Category?	BO
BC breaks?	no
Deprecations?	no
Fixed ticket?	This was discussed in #12585
How to test?	Explained below.

To test:

Install this module: module
The module will add two new fields in the bottom of language edit form.

Without this PR the fields are unaligned as we cannot control the rendering: https://prnt.sc/nc96pk
With this PR it should look much better, since we can customize the rendering: https://prnt.sc/nc9zjy

This change is

rokaszygmantas · 2019-04-15T15:35:52Z

src/PrestaShopBundle/Resources/views/Admin/TwigTemplateForm/form_div_layout.html.twig

@@ -320,7 +320,7 @@

 {%- block form_end -%}
  {%- if not render_rest is defined or render_rest -%}
-    {{ form_rest(form) }}
+    {{ form_rest(form, { render_hook: false }) }}


this prevents form_rest() from rendering the hook twice.

I'm wondering maybe we should do the opposite to avoid side effects?
Also if we do it this way, does this mean if you forget to add form_rest in the form the hook won't be rendered?

I'm wondering maybe we should do the opposite to avoid side effects?

what do you mean by that?

if you forget to add form_rest in the form the hook won't be rendered?

yes

I'm wondering maybe we should do the opposite to avoid side effects?

what do you mean by that?

I meant instead of disabling the feature with the parameter we could use form_rest(form, { render_hook: true }) instead and explicitly enable it so that if we use a simple form_rest(form) the hook is not rendered

if you forget to add form_rest in the form the hook won't be rendered?

yes

ok, so we will have to modify every existing form to add form_rest if we want to add the hook?

meant instead of disabling the feature with the parameter we could use form_rest(form, { render_hook: true }) instead and explicitly enable it so that if we use a simple form_rest(form) the hook is not rendered

The idea was to make it render the hook automatically whenever form_rest is used, but maybe it's smart not render it automatically and control it with a parameter 🤔 IMO one more opinion would be great, ping @matks @sarjon

ok, so we will have to modify every existing form to add form_rest if we want to add the hook?

yes. Also, every form should have a form_rest already rendered, unless developers missed it (usually by mistake)

Well here seems like form_rest is called automatically by form_end

So if I understand the current implementation, if you didn't forget to add form_rest in your form, it will be called one time with the hook (by default) and a second time by form_end (without the hook)

And of course you still have the possibility to prevent the hook in your form with an explicit {{ form_rest(form, { render_hook: false }) }} This is a bit clearer for me, sorry I wasn't sure about the behavior

It seems to me that this approach is the more user friendly and the less intrusive at the same time, it's a good compromise. But as you said let's confirm it with the others and maybe @eternoendless when he comes bak from holiday (end of the week)

rokaszygmantas · 2019-04-15T15:38:05Z

src/PrestaShopBundle/Resources/views/Admin/TwigTemplateForm/form_div_layout.html.twig

@@ -340,6 +340,10 @@
 {%- endblock form_errors -%}

 {%- block form_rest -%}
+  {% if render_hook is not defined or render_hook %}
+    {{ renderhook('display' ~ name ~ 'FormRest', { form: form }) }}


there might be a problem with forms that don't have custom names as the hook name will be not unique.
For example, many options forms have "form" as form name.

Indeed this is something we need to change

We should camelize the form name to avoid bugs like #13594 and #13580

By the no need to call parent() or something in this block?

Forget this last comment, I didn't see that children were rendered after

tomas862 · 2019-04-16T10:28:01Z

@matks is this PR should go to develop branch? It would be perfect that it would appear in 1.7.6 due to the sample module I am creating would use this hook to demonstrate how forms are being override.

Without this I think the sample module will only have the demonstration of overriding the listing only and not the forms since it will not look okey with current implementation.

matks · 2019-04-16T12:39:50Z

@matks is this PR should go to develop branch? It would be perfect that it would appear in 1.7.6 due to the sample module I am creating would use this hook to demonstrate how forms are being override.

Without this I think the sample module will only have the demonstration of overriding the listing only and not the forms since it will not look okey with current implementation.

What do you mean but "not look OK" ? Inputs really look bad, or just up to PrestaShop UX quality ?
Inputs are supposed to be displayed following our FormTheme by default. I know it needs improvement but it is not that bad, is it ?

This PR would be a lot better in develop 😐 as it is not a bugfix.

tomas862 · 2019-04-16T12:52:33Z

@matks is this PR should go to develop branch? It would be perfect that it would appear in 1.7.6 due to the sample module I am creating would use this hook to demonstrate how forms are being override.
Without this I think the sample module will only have the demonstration of overriding the listing only and not the forms since it will not look okey with current implementation.

What do you mean but "not look OK" ? Inputs really look bad, or just up to PrestaShop UX quality ?
Inputs are supposed to be displayed following our FormTheme by default. I know it needs improvement but it is not that bad, is it ?

This PR would be a lot better in develop neutral_face as it is not a bugfix.

@matks I mean they will look like this https://prnt.sc/nc96pk - they look good but they are not aligned with the rest of the content since right now I can't control the html from the hook. I only able to add new form type and the form_rest will render it.

If I recall correctly even the form_rest in some pages are misslocated so new inputs will appear in random places like page bottom in some forms.

matks · 2019-04-16T12:58:16Z

@matks is this PR should go to develop branch? It would be perfect that it would appear in 1.7.6 due to the sample module I am creating would use this hook to demonstrate how forms are being override.
Without this I think the sample module will only have the demonstration of overriding the listing only and not the forms since it will not look okey with current implementation.

What do you mean but "not look OK" ? Inputs really look bad, or just up to PrestaShop UX quality ?
Inputs are supposed to be displayed following our FormTheme by default. I know it needs improvement but it is not that bad, is it ?
This PR would be a lot better in develop neutral_face as it is not a bugfix.

@matks I mean they will look like this https://prnt.sc/nc96pk - they look good but they are not aligned with the rest of the content since right now I can't control the html from the hook. I only able to add new form type and the form_rest will render it.

If I recall correctly even the form_rest in some pages are misslocated so new inputs will appear in random places like page bottom in some forms.

Interesting 🤔 but now I'm sure it's really due to our Form Theme (so #11389) rather than form_rest() being unhookable.

Bug bad alignment is not an issue bad enough to make us bend the code freeze rules 😉 .

Let's follow Beck's rule : 1) Make it work 2) Make it right 3) Make it fast. Let's finish this PR (with badly aligned inputs) to solve 1) then improve the Form theme to solve 2) . We'll see with eternoendless if a bad form theme is considered a bug for 1.7.6 or not 😉

matks · 2019-04-18T14:48:23Z

@rokaszygmantas Well it looks promising 👍 actually it's not a POC, this is working right ?

@Quetzacoalt91 @jolelievre @eternoendless any feedback on this ? can enable some very interesting customization capabilities for modules

rokaszygmantas · 2019-04-18T14:49:29Z

@matks yeah it's a working POC 😄

matks · 2019-04-29T14:19:41Z

@jolelievre @PierreRambaud @eternoendless I'd like a 2nd opinion before merge but I really like this idea / PR 😄

rokaszygmantas · 2019-06-04T05:52:37Z

@matks what's needed for this PR to be merged? besides rebase of course 🙂

matks · 2019-06-12T15:54:36Z

@matks what's needed for this PR to be merged? besides rebase of course 🙂

I'm waiting to see how #14094 turns out to be sure there's no conflicts

rokaszygmantas · 2019-07-24T10:10:19Z

@matks should I close this PR? 🙂

matks · 2019-08-26T12:42:24Z

OK, back to this PR now that we have been able to see how #14094 turned out. Also time helps to have a bigger view of the issue.

Good news: as @sarjon has started doing, we're going to render form usiong form(form) (see #15130). This is going to make this hook even more useful.

For this PR, here's what we need to check

if all forms are rendered using form(form) should we move the hook dispatch inside form(form), this way hijacking Symfony way of rendering form so we allow modules to fully control the rendering of any migrated pages without overriding templates ?
if yes, do we use one hook for form_rest() too ? this can be useful if a twig custom theme is used and form(form) is not available ?
we need to document it, of course

Waiting for input @rokaszygmantas @sarjon @eternoendless 😄

sarjon · 2019-08-26T12:48:54Z

I think whether we need this PR or not depends on decision made here #15130 (comment) 🤔

matks · 2019-09-13T09:18:04Z

@eternoendless I think if we can render all forms using form(form) this PR becomes useless.

Question now is: is dispatching a hook into form(form) is useful 🤔?

A module that wishes to customize the behavior of a form can already do it by overriding twig template. However this means 2 modules cannot override the same template. But is it a good thing, to allow multiple customization ? Especially now that we have CustomContentType

matks · 2020-01-23T14:26:39Z

I think if we can render all forms using form(form) this PR becomes useless.

I think now, we know the answer is: indeed this PR is now useless. Same customisation capabilities (even better !) will be provided by #16891

Add a hook to form_rest for custom field rendering

2ce8b1a

prestonBot added develop Branch Feature Type: New Feature labels Apr 15, 2019

rokaszygmantas commented Apr 15, 2019

View reviewed changes

matks added the migration symfony migration project label Apr 16, 2019

tomas862 mentioned this pull request Apr 19, 2019

Issues when creating new module for PS 1.7.6 #13490

Closed

5 tasks

matks mentioned this pull request May 10, 2019

Make symfony forms easier to extend from modules using form_rest #12585

Closed

matks added the Waiting for author Status: action required, waiting for author feedback label Aug 26, 2019

matks closed this Jan 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[POC] Add a hook to form_rest for custom field rendering #13412

[POC] Add a hook to form_rest for custom field rendering #13412

rokaszygmantas commented Apr 15, 2019 •

edited by eternoendless

rokaszygmantas Apr 15, 2019

jolelievre Apr 29, 2019

rokaszygmantas Apr 30, 2019

jolelievre Apr 30, 2019

rokaszygmantas May 6, 2019

jolelievre May 6, 2019

rokaszygmantas Apr 15, 2019

matks Apr 18, 2019

jolelievre Apr 29, 2019

jolelievre Apr 29, 2019

jolelievre Apr 30, 2019

tomas862 commented Apr 16, 2019

matks commented Apr 16, 2019

tomas862 commented Apr 16, 2019

matks commented Apr 16, 2019

matks commented Apr 18, 2019

rokaszygmantas commented Apr 18, 2019

matks commented Apr 29, 2019

rokaszygmantas commented Jun 4, 2019

matks commented Jun 12, 2019

rokaszygmantas commented Jul 24, 2019

matks commented Aug 26, 2019

sarjon commented Aug 26, 2019

matks commented Sep 13, 2019

matks commented Jan 23, 2020

[POC] Add a hook to form_rest for custom field rendering #13412

[POC] Add a hook to form_rest for custom field rendering #13412

Conversation

rokaszygmantas commented Apr 15, 2019 • edited by eternoendless

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomas862 commented Apr 16, 2019

matks commented Apr 16, 2019

tomas862 commented Apr 16, 2019

matks commented Apr 16, 2019

matks commented Apr 18, 2019

rokaszygmantas commented Apr 18, 2019

matks commented Apr 29, 2019

rokaszygmantas commented Jun 4, 2019

matks commented Jun 12, 2019

rokaszygmantas commented Jul 24, 2019

matks commented Aug 26, 2019

sarjon commented Aug 26, 2019

matks commented Sep 13, 2019

matks commented Jan 23, 2020

rokaszygmantas commented Apr 15, 2019 •

edited by eternoendless