October: Static Page Repeater Groups Save Failure

[seanthepottingshed]

• OctoberCMS Build: v1.0.451
• PHP Version: 7.1.17
• Database Engine: mysql
• Plugins Installed: RainLab.Pages v1.2.20

Description:

Saving Repeater with Groups in a Static Page doesn't work as _group property is empty.

Steps To Reproduce:

• Install a fresh version of OctoberCMS
• Install RainLab.Pages
• Create a Static Page called home.htm with a URL of /
• Install the following PR: https://github.com/octoberrain/test-plugin/pull/65
• Reload / Static Page in CMS
• Save Static Page
• Inspect themes/demo/content/static-pages/home.htm you will see in the viewBag that the _group properties are empty, eg:

[viewBag]
title = "Test"
url = "/"
is_hidden = 0
navigation_hidden = 0
test_repeater[0][text_area] = "Test"
test_repeater[0][_group] = ""
==

[LukeTowers]

Wow, there are so many choices on how to fix this bug.

1. Remove uniqid() from the form widget alias generation in the RainLab.Pages plugin: https://github.com/rainlab/pages-plugin/blob/master/controllers/Index.php#L397
   Pros: seems to be pointless and buggy to have that there given that the form alias will change on every single request which renders it useless if attempting to use it in input names
   Cons: Not sure why it was originally added, could break stuff maybe? Doubt it.

2. Revert https://github.com/octobercms/october/commit/3954704ddaf0e08acb8ec26d14d4b0e455f0d1f4#diff-a8e657fa9654328055121a4e2cc4311b
   Pros: Would fix the issue?
   Cons: Would break other shit

3. Remove https://github.com/octobercms/october/blob/develop/modules/backend/formwidgets/Repeater.php#L191
   Pros: Would simplify the logic of the Repeater a lot if we just used the _group sent in each item's row data instead of all this mucking about with groupInputName duplication of the group code, and potentially get rid of indexInputName at the same time
   Cons: No idea why groupInputName and indexInputName exist in the first place, so unsure if it would be causing more bugs

I'm personally a fan of 1 & 3 combined.

[LukeTowers]

@seanthepottingshed @bennothommo any input on the above?

[seanthepottingshed]

@LukeTowers 3 sounds like a winner to me; however @bennothommo would be in better position than me to understand any caveats associated with this approach. Also need to consider the impact this may have on the unresolved issue with Repeater Group deletion in CMS pages: https://github.com/octobercms/october/issues/4175#issuecomment-469172078

[bennothommo]

@LukeTowers @seanthepottingshed I'll aim to have a look this evening and give my two cents worth. :)

[bennothommo]

@LukeTowers @seanthepottingshed Regarding option 3, whilst removing the group fields outside of the item row data is probably safe, I believe the reason for the extra index fields is to keep track of ordering. I believe some JSON implementations (especially in the JavaScript side) will reorder numbered sequential arrays based on their keys, which means data like this:

Country[content][1][text_area]: Test
Country[content][1][_group]: textarea
formContent___index_content[]: 1
formContent___group_content[]: textarea
Country[content][3][img_upload]: 
Country[content][3][_group]: image
formContent___index_content[]: 3
formContent___group_content[]: image
Country[content][2][quote_position]: left
Country[content][2][quote_content]: Test
Country[content][2][_group]: quote
formContent___index_content[]: 2
formContent___group_content[]: quote

... will result in the 3rd content item being listed third, even though it is second in the POST data. If the numbered key was changed to a string (ie: i1, i2, i3), that would work fine, but would need extra processing.

[LukeTowers]

@bennothommo could you test out my latest commit? https://github.com/octobercms/october/commit/b4b4b1b566d9c117896d7c24ae835d0be2f916c0

[seanthepottingshed]

@LukeTowers

I've tested your commit and this resolves my issue.

[seanthepottingshed]

@LukeTowers @bennothommo

Not that I was expecting it to - but I've also tested to see if this resolves the CMS Repeater Group deletion issue, and that issue remains.

[bennothommo]

@seanthepottingshed I let @LukeTowers know via Slack that his fix seemed to have broken the repeater in the October Test plugin in the Countries form under the Content tab. That one uses groups in its config as well.

[LukeTowers]

@bennothommo could you test it again? I'm unable to replicate your issue. @seanthepottingshed are you talking about my option 1 commit (remove uniqid()) or my option 3 (remove extra __index_, __group_ processing)?

[seanthepottingshed]

@LukeTowers

I was referring to this: https://github.com/octobercms/october/commit/b4b4b1b566d9c117896d7c24ae835d0be2f916c0

I'm off to Scotland for a few days will be back to work on Monday, have a fantastic weekend!