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

[drzraf]

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]

ping

[mahagr]

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]

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.

[drzraf]

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]

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]

@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]

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

[drzraf]

?!

[drzraf]

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]

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