improve route-based selection when multiple partial forms have the same slug #309

drzraf · 2018-12-01T21:47:56Z

When a form is requested by route, the routine extract the matching one then get its name, and then fetch form by name.
Not only is this sub-optimal but create a bug when two forms share the same name.

Eg: a form is used as part of a modular page and dynamically included in the template by its name. If another modular page exists, containing a page with an identical form slug, then this will fail.

=> In case a route is requested and form exists: just return it immediately.

drzraf · 2019-01-23T16:44:33Z

ping

mahagr · 2019-02-15T20:46:06Z

I ran into the same issue when I was redoing the form plugin.

Because of that, I have actually made some improvements for this in Form 3.0 (in the testing channel), making it to check the current page first and to include forms from modular content in the modular page.

Can you test it out?

drzraf · 2019-02-19T22:24:46Z

I believe you're referring to 1f07211 and d2d2130
I can hardly cherry-pick to individually test them. A lot changed in the meantime. I'll try to manually backport to our fork. The fact that it's reserved to >= Grav 1.6 makes things a bit harder.
Please note that, as advertised, this fixes lays in the getForm function and avoid an indirection. Independently for bug fixing, the "optimisation/simplication" can still makes sense in 3.0.

then get its name, and then fetch form by name. Not only is this suboptimal but create a bug when two forms share the same name. Eg: a form is used as part of a modular page and dynamically included in the template by its name. If another modular page exists, containing a page with an identical form slug, then this will fail. => In case route is requested and form exists: just return it inmediatly getgrav#309

drzraf · 2019-02-19T22:50:38Z

I installed Grav 1.6 and tried forms 3.0 : it does not make it.
Let's say you have two modular pages: 2016, including a form named foobar, and 2018 including a form named foobar. Then both pages will show foobar from 2018 (in my case 2018 is my homepage). This is not right nor expected. 2016 should not consider using forms defined in the 2018 modular page (unless explicitly requested). The extra-indirection is not right w.r.t this and that's what this patch drops (rebased on 3.0)

mahagr · 2019-02-20T10:34:46Z

OK, I took a deeper look into the logic to fetch the form and I don't think your fix is enough to solve the whole issue. The logic is written to find forms from the global namespace and not from individual pages. I don't think its correct -- forms in the current page should always take precedence and only if there aren't any forms, should it look for other pages.

@rhukster Changing this behaviour may radically change which forms get selected, but right now the logic makes many advanced uses of the forms impossible. Prioritizing the current page would make Flex logic much simpler, for example, because of I could get rid of some of the custom logic in there.

drzraf · 2019-02-20T12:44:54Z

@mahagr I completely agree that form selection should be made much more clear.
I updated this fix to conserve name-based selection in case no form object is available.
What it does is avoiding to run through (global) name-fetching when the form object exists which is, I think, what we desire about precedence. Why do you think it's not enough?

mahagr · 2019-02-21T07:48:45Z

I'll discuss with @rhukster on this. We need to be sure we're not breaking anything that's not broken.

then get its name, and then fetch form by name. Not only is this suboptimal but create a bug when two forms share the same name. Eg: a form is used as part of a modular page and dynamically included in the template by its name. If another modular page exists, containing a page with an identical form slug, then this will fail. => In case route is requested and form exists: just return it inmediatly getgrav#309

drzraf · 2019-04-18T13:21:08Z

?!

drzraf · 2019-04-18T23:06:57Z

I fear I've to repeat once more.
This simple PR DOES fix an actual and existing bug.

create two modular pages, each including a form, eg:
- 01.aaaa/01._formular/form.de_DE.md: title: form-for-aaaa
- 02.bbbb/01._formular/form.de_DE.md: title: form-for-bbbb
You'll encounter that 01.aaaa is using 02.bbbb's form or vice-and-versa !!

I think that unless someone proves that the existing line-code serves a real purpose and fixes a bug, the buggy-line must be removed.
It may radically change which forms get selected but my guess it rather that wrong forms are currently selected.

mahagr · 2019-04-23T07:48:14Z

I believe the issue was closed because of the branch does not exist anymore. @drzraf Please do the change against develop

then get its name, and then fetch form by name. Not only is this suboptimal but create a bug when two forms share the same name. Eg: a form is used as part of a modular page and dynamically included in the template by its name. If another modular page exists, containing a page with an identical form slug, then this will fail. => In case route is requested and form exists: just return it inmediatly getgrav#309

…mine/form-attr-improvments Squashed commit of the following: commit 5f9fcae Author: Raphaël Droz <raphael.droz+floss@gmail.com> Date: Tue Feb 19 19:43:24 2019 -0300 When a form is requested by route, the routine extract the matching one then get its name, and then fetch form by name. Not only is this suboptimal but create a bug when two forms share the same name. Eg: a form is used as part of a modular page and dynamically included in the template by its name. If another modular page exists, containing a page with an identical form slug, then this will fail. => In case route is requested and form exists: just return it inmediatly getgrav#309 commit aac0df0 Author: Raphaël Droz <raphael.droz+floss@gmail.com> Date: Thu Dec 20 12:02:30 2018 -0300 Show the `description` span even for an empty description because this field, even if empty at initial page load may be used and changed by javascript listeners. commit d83422d Author: Raphaël Droz <raphael.droz+floss@gmail.com> Date: Fri Nov 16 10:38:53 2018 -0300 allow form scope to be set from form parameters allow data-* form parameters to be used as <form> attributes (blueprints-expansion enabled)

…ne (#338) then get its name, and then fetch form by name. Not only is this suboptimal but create a bug when two forms share the same name. Eg: a form is used as part of a modular page and dynamically included in the template by its name. If another modular page exists, containing a page with an identical form slug, then this will fail. => In case route is requested and form exists: just return it inmediatly #309

…mine/form-attr-improvments Squashed commit of the following: commit 5f9fcae Author: Raphaël Droz <raphael.droz+floss@gmail.com> Date: Tue Feb 19 19:43:24 2019 -0300 When a form is requested by route, the routine extract the matching one then get its name, and then fetch form by name. Not only is this suboptimal but create a bug when two forms share the same name. Eg: a form is used as part of a modular page and dynamically included in the template by its name. If another modular page exists, containing a page with an identical form slug, then this will fail. => In case route is requested and form exists: just return it inmediatly getgrav#309 commit aac0df0 Author: Raphaël Droz <raphael.droz+floss@gmail.com> Date: Thu Dec 20 12:02:30 2018 -0300 Show the `description` span even for an empty description because this field, even if empty at initial page load may be used and changed by javascript listeners. commit d83422d Author: Raphaël Droz <raphael.droz+floss@gmail.com> Date: Fri Nov 16 10:38:53 2018 -0300 allow form scope to be set from form parameters allow data-* form parameters to be used as <form> attributes (blueprints-expansion enabled)

drzraf force-pushed the same-slug-forms branch 2 times, most recently from f184330 to bfa102e Compare December 1, 2018 22:04

drzraf force-pushed the same-slug-forms branch from bfa102e to 3cf7bd2 Compare February 19, 2019 22:46

drzraf changed the base branch from develop to 3.0 February 19, 2019 22:47

Merge branch '3.0' into same-slug-forms

efa9503

w00fz closed this Apr 17, 2019

drzraf mentioned this pull request Apr 24, 2019

Prefer forms matching by route to avoid wrong result in case of name collision (was #309) #338

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve route-based selection when multiple partial forms have the same slug #309

improve route-based selection when multiple partial forms have the same slug #309

drzraf commented Dec 1, 2018

drzraf commented Jan 23, 2019

mahagr commented Feb 15, 2019

drzraf commented Feb 19, 2019 •

edited

Loading

drzraf commented Feb 19, 2019 •

edited

Loading

mahagr commented Feb 20, 2019

drzraf commented Feb 20, 2019

mahagr commented Feb 21, 2019

drzraf commented Apr 18, 2019

drzraf commented Apr 18, 2019

mahagr commented Apr 23, 2019

improve route-based selection when multiple partial forms have the same slug #309

improve route-based selection when multiple partial forms have the same slug #309

Conversation

drzraf commented Dec 1, 2018

drzraf commented Jan 23, 2019

mahagr commented Feb 15, 2019

drzraf commented Feb 19, 2019 • edited Loading

drzraf commented Feb 19, 2019 • edited Loading

mahagr commented Feb 20, 2019

drzraf commented Feb 20, 2019

mahagr commented Feb 21, 2019

drzraf commented Apr 18, 2019

drzraf commented Apr 18, 2019

mahagr commented Apr 23, 2019

drzraf commented Feb 19, 2019 •

edited

Loading

drzraf commented Feb 19, 2019 •

edited

Loading