Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create possibility for nested subforms #17552

Merged
merged 3 commits into from Sep 22, 2018

Conversation

@continga
Copy link
Contributor

commented Aug 15, 2017

Pull Request for Issue #11551

Summary of Changes

This PR adds the possibility to nest subforms into subforms, adding the possibility to stack them as many times as a user would like to.

Testing Instructions

Create a form with a subform inside of a subform and test whether it works as expected (with multiple=true or false, layout=repeatable or repeatable-table, different min/max values, etc...).

A simple testcase is to testwise edit e.g. plugins/fields/checkboxes/params/checkboxes.xml, adding e.g. the following after line 27:

<field
	name="subformlevel2"
	type="subform"
	label="PLG_FIELDS_CHECKBOXES_PARAMS_OPTIONS_LABEL"
	description="PLG_FIELDS_CHECKBOXES_PARAMS_OPTIONS_DESC"
	layout="joomla.form.field.subform.repeatable-table"
	icon="list"
	multiple="true"
>
	<form hidden="true" name="list_templates_modal" repeat="true">
		<field
			name="name_lvl2"
			type="text"
			label="PLG_FIELDS_CHECKBOXES_PARAMS_OPTIONS_NAME_LABEL"
			size="30"
		/>
	</form>
</field>

After that has been added, navigate to Content -> Fields -> New in the Administrator Backend, select "Checkboxes" as Type, and play around with the nested subforms.

To do some additional tests, play around with the XML definition (adding 3rd level subforms, changing the multiple/non-multiple stuff, required status, etc...) and take a look whether everything works as expected.

Expected result

Something like in the following screenshot, where I have stacked 3 subforms inside of each other with just some basic textfields:
image

Additional information

This is a backwards-compatible change and hence does not require anything to be worried about (imho) - please correct me if I am wrong, maybe we can find a solution then.

I am aware that the main merge window of Joomla! 3.8 already has passed, so I guess this PR probably will target version 3.9 or 4.0, but I open this PR now because it is my first slightly bigger PR for Joomla, and I expect some feedback-cycles before we reach a merge-ready state.

I already have a question to begin with: I only committed the uncompressed JS file, is it my job to do the minification, or will some deploy-script take care of that?

Thanks in advance!

@brianteeman

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2017

I already have a question to begin with: I only committed the uncompressed JS file, is it my job to do the minification, or will some deploy-script take care of that?

For Joomla 3.x then yes please. You can use any minifier

@brianteeman

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2017

@continga there are several codestyle errors that have been reported here http://213.160.72.75/joomla/joomla-cms/7879 that it would be great if you could resolve - thanks

@laoneo

This comment has been minimized.

Copy link
Member

commented Aug 16, 2017

Cool feature, was waiting for it!! Does that also work with the default layout?

// separate unique subform id present to could distinguish the eventhandlers
// regarding adding/moving/removing rows from nested subforms from their parents.
static $unique_subform_id = 0;
$data['unique_subform_id'] = ('sr-' . $unique_subform_id++);

This comment has been minimized.

Copy link
@Fedik

Fedik Aug 17, 2017

Contributor

name confusing, technically it is counter,
each field already have an ID $this->id, which is unique
just IMHO 😉

but seems not a bad idea to resolve "buttons" conflicts 😉

<?php /** Do a rawurlencode to not tamper with HTML elements, especially with
* nested subforms (subform in subform), this might contain a
* </script> tag, which else blows up our markup. */ ?>
<?php echo rawurlencode(trim($this->sublayout(

This comment has been minimized.

Copy link
@Fedik

Fedik Aug 17, 2017

Contributor

I do not like it, and I would not accept it with rawurlencode. why? 😉

The problem is correct, we cannot have nested <script> tag.
But solution not the best.

I had idea to use <template> tag, wich can be nested. That would be a correct solution.
But need to search/test for <template> polyfill to make it work in IE.

This comment has been minimized.

Copy link
@dgrammatiko

dgrammatiko Aug 17, 2017

Contributor

Big thumbs up for template, in J4 we have a polyfill already!
I think another good idea is to move all the field specific scripts to custom elements. The advantage is pretty obvious, we define a class one and use it as many times as we want and also less dom (slow) queries...

}
// Recursively replace our basename + old group with basename + new group
// inside of nested subform template elements.
this.recursiveReplaceNested($row, search, replace);

This comment has been minimized.

Copy link
@Fedik

Fedik Aug 17, 2017

Contributor

hm, I have not tested but it looks not safe.
Each instance of subformRepeatable should work only with "own" fields, and should not care about nested/parent etc.

Additionally, you have fixed only the input name,
but it also require to fix an input id and label for.

This is one of complex issue with nested subform 😉

This comment has been minimized.

Copy link
@Fedik

Fedik Aug 17, 2017

Contributor

hm, I see, you fixed only nested template,
but the inputs attributes of this template will be calculated depend from data-base-name, data-group, later, when subform add new row

@Fedik

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2017

@continga thanks for doing it, it is often requested feature 😉

@continga continga force-pushed the continga:issue-11551 branch from cb61832 to a26269c Aug 31, 2017

@laoneo laoneo referenced this pull request Sep 5, 2017

Closed

Multi level subform #11551

@laoneo

This comment has been minimized.

Copy link
Member

commented Sep 5, 2017

Is this ready for testing? Because I tested it and it worked, but the minified script is missing. I really would like to get that in.

@continga

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2017

I had some other things to do the last 2 weeks, but will continue to work on this soon to include the feedback from above and fix the CI fails

@continga continga force-pushed the continga:issue-11551 branch from a26269c to c449f1f Sep 12, 2017

@continga

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2017

I have now included all your feedback (I at least hope so ;-)). Would be nice if you guys now could take another look. Thanks!

@laoneo

This comment has been minimized.

Copy link
Member

commented Sep 13, 2017

I have tested this item successfully on f485db8


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17552.

@drmenzelit

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2017

I have tested this item successfully on f485db8

Tested successfully with 3 level subforms like the example


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17552.

@continga

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2017

Is there anything we can do for merging this PR. If it's merged we like to go on with more PRs to add repeatable subforms to custom fields.

@franz-wohlkoenig

This comment has been minimized.

Copy link
Member

commented Oct 12, 2017

RTC after two successful tests.

Thanks for Hint, @continga

@joomla-cms-bot joomla-cms-bot added the RTC label Oct 12, 2017

@continga continga force-pushed the continga:issue-11551 branch from f485db8 to 7c943d7 Nov 7, 2017

*/
var nestedTemplates = $row.find(this.options.rowTemplateSelector);
// If we found it, iterate over the found ones (might be more than one!)
for (var i = 0; i < nestedTemplates.length; i++) {

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Nov 7, 2017

'i' is already defined.

) {
var group = (typeof group === 'undefined' ? $row.attr('data-group') : group),
basename = (typeof basename === 'undefined' ? $row.attr('data-base-name') : basename),
count = (typeof count === 'undefined' ? 0 : count),

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Nov 7, 2017

'count' is already defined.

basename // group base name, without count, e.g. 'options'
) {
var group = (typeof group === 'undefined' ? $row.attr('data-group') : group),
basename = (typeof basename === 'undefined' ? $row.attr('data-base-name') : basename),

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Nov 7, 2017

'basename' is already defined.

group, // current group name, e.g. 'optionsX'
basename // group base name, without count, e.g. 'options'
) {
var group = (typeof group === 'undefined' ? $row.attr('data-group') : group),

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Nov 7, 2017

'group' is already defined.

continga pushed a commit to continga/joomla-cms that referenced this pull request Nov 7, 2017

continga pushed a commit to continga/joomla-cms that referenced this pull request Nov 7, 2017

@mbabker mbabker added this to the Joomla 3.9.0 milestone Nov 24, 2017

@tonypartridge

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2017

@mbabker why the 3.9 release? Can we get this in 3.8.4 please since it’s a big fix as there is nothing to say that it can only be one level deep? It should be multiple and therefore a bug.

@franz-wohlkoenig

This comment has been minimized.

Copy link
Member

commented Dec 19, 2017

@tonypartridge as far as i know: new Features are shipped with new Release like 3.9., 3.8.* are for Fixes.

@tonypartridge

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2017

@franz-wohlkoenig

This comment has been minimized.

Copy link
Member

commented Dec 19, 2017

ah, thought its a new Feature (Label).

@joomla-cms-bot joomla-cms-bot removed the RTC label Sep 17, 2018

@continga

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2018

@ReLater Thank you for testing, bug is fixed now!

@tonypartridge

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2018

I have tested this item successfully on cdbe0c9

Working well


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17552.

I modified a module, added a form to /forms/ then a subsub form to forms/

Included and run before patch, repeating field goes crazy.

Include and run after patch, forms are respectable, + and x toggles work as expected for each form.

Tested saving, saves as expected and reloads with the data passed into the form.

@continga

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2018

@ReLater can you re-test this? If you give your GO we have two successful test 🙂 😊

@carcam

This comment has been minimized.

Copy link

commented Sep 19, 2018

I have tested this item successfully on cdbe0c9

I have successfully tested this patch in Joomla! 3.8.12 and 3.9 beta with Patch tester.
I have successfully tested this path directly from the commit cbe0c9 code.

Test performed:

I have added this field to one of my components configuration:

<field
	name="subformlevel2"
	type="subform"
	label="PLG_FIELDS_CHECKBOXES_PARAMS_OPTIONS_LABEL"
	description="PLG_FIELDS_CHECKBOXES_PARAMS_OPTIONS_DESC"
	layout="joomla.form.field.subform.repeatable-table"
	icon="list"
	multiple="true"
>
    <form hidden="true" name="list_templates_modal" repeat="true">
	<field
		name="name_lvl2"
		type="text"
		label="PLG_FIELDS_CHECKBOXES_PARAMS_OPTIONS_NAME_LABEL"
		size="30"
		/>
	<field
		name="subformlevel3"
		type="subform"
		label="PLG_FIELDS_CHECKBOXES_PARAMS_OPTIONS_LABEL"
		description="PLG_FIELDS_CHECKBOXES_PARAMS_OPTIONS_DESC"
		layout="joomla.form.field.subform.repeatable"
		icon="list"
		multiple="true"
	>
	<form hidden="true" name="third_level" repeat="true">
	<field
		name="name_lvl3"
		type="text"
		label="PLG_FIELDS_CHECKBOXES_PARAMS_OPTIONS_NAME_LABEL"
		size="30"
		/>
	</form>
	</field>
    </form>
</field>

and in all scenarios the patch has worked with no issues.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17552.

@franz-wohlkoenig

This comment has been minimized.

Copy link
Member

commented Sep 19, 2018

Ready to Commit after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC label Sep 19, 2018

@mbabker mbabker added this to the Joomla 3.9.0 milestone Sep 22, 2018

@mbabker mbabker merged commit f2a9874 into joomla:staging Sep 22, 2018

5 checks passed

Hound No violations found. Woof!
JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joomla-cms-bot joomla-cms-bot removed the RTC label Sep 22, 2018

continga pushed a commit to continga/joomla-cms that referenced this pull request Sep 24, 2018

Rene Pasing
This is a combination of 19 commits.
This is the 1st commit message:

First WIP version of supporting nested subforms.

This is the commit message joomla#2:

Fix nested subforms not getting correct name attribute on input fields.

This is the commit message joomla#3:

Fix subform rows having invalid index, fix small typo.

This is the commit message joomla#4:

Replace the unique subform id via random bytes by just an increasing integer in the fields type rendering process.

This is the commit message joomla#5:

Implement feedback from PR at Joomla;
- Use a <template> HTML element for the template of the subform rows,
  not a url encoded string inside of a <script> element.
- Fix code style errors reported by phpcs.
- Make the fixing of the unique attributes (name, id, etc) of input elements
  of nested subform rows more errorprone, using the same method as the main
  subform row.
- Manually add a minified version of the javascript file.

This is the commit message joomla#6:

Fix failing javascript tests due to changed structure of subform repeatable template container.

This is the commit message joomla#7:

Change subform repeatable javascript test to correctly check on
0-indexed rows, previously they have been 1-indexed.

This is the commit message joomla#8:

Fix a problem where multi-level subforms on the same level doesnt trigger their template correctly.
Additionally added a note why the fixScripts() method is broken and how it could get better.

This is the commit message joomla#9:

First commit which integrates the subform custom field type.

This is the commit message joomla#10:

Rename manifest file for plg_fields_subform

This is the commit message joomla#11:

plg_fields_subform: Improve code comments and made the subform field type optionally non-repeatable.

This is the commit message joomla#12:

Fix not being able to change the type of a subfield.

This is the commit message joomla#13:

Implement a small in-memory renderCache for subform custom fields

This is the commit message joomla#14:

Prepare to load the subfields into a subform custom field directly via their own xml definition.

This is the commit message joomla#15:

After merge with PR joomla#17552 we can now successfully show the subforms options for the different types.

This is the commit message joomla#16:

The subform custom field type doesnt need a default value.

This is the commit message joomla#17:

Implement an option for a subform customfield to disable rendering the field values.

This is the commit message joomla#18:

Remove unused code and set hidden fields to not-required.

This is the commit message joomla#19:

Reset for dev

continga pushed a commit to continga/joomla-cms that referenced this pull request Sep 24, 2018

This is a combination of 10 commits.
This is the 1st commit message:

First commit which integrates the subform custom field type.

This is the commit message joomla#2:

Rename manifest file for plg_fields_subform

This is the commit message joomla#3:

plg_fields_subform: Improve code comments and made the subform field type optionally non-repeatable.

This is the commit message joomla#4:

Fix not being able to change the type of a subfield.

This is the commit message joomla#5:

Implement a small in-memory renderCache for subform custom fields

This is the commit message joomla#6:

Prepare to load the subfields into a subform custom field directly via their own xml definition.

This is the commit message joomla#7:

After merge with PR joomla#17552 we can now successfully show the subforms options for the different types.

This is the commit message joomla#8:

The subform custom field type doesnt need a default value.

This is the commit message joomla#9:

Implement an option for a subform customfield to disable rendering the field values.

This is the commit message joomla#10:

Remove unused code and set hidden fields to not-required.

continga pushed a commit to continga/joomla-cms that referenced this pull request Sep 24, 2018

Rene Pasing
This is a combination of 10 commits.
This is the 1st commit message:

First commit which integrates the subform custom field type.

This is the commit message joomla#2:

Rename manifest file for plg_fields_subform

This is the commit message joomla#3:

plg_fields_subform: Improve code comments and made the subform field type optionally non-repeatable.

This is the commit message joomla#4:

Fix not being able to change the type of a subfield.

This is the commit message joomla#5:

Implement a small in-memory renderCache for subform custom fields

This is the commit message joomla#6:

Prepare to load the subfields into a subform custom field directly via their own xml definition.

This is the commit message joomla#7:

After merge with PR joomla#17552 we can now successfully show the subforms options for the different types.

This is the commit message joomla#8:

The subform custom field type doesnt need a default value.

This is the commit message joomla#9:

Implement an option for a subform customfield to disable rendering the field values.

This is the commit message joomla#10:

Remove unused code and set hidden fields to not-required.

continga pushed a commit to continga/joomla-cms that referenced this pull request Sep 24, 2018

Rene Pasing
This is a combination of 10 commits.
This is the 1st commit message:

First commit which integrates the subform custom field type.

This is the commit message joomla#2:

Rename manifest file for plg_fields_subform

This is the commit message joomla#3:

plg_fields_subform: Improve code comments and made the subform field type optionally non-repeatable.

This is the commit message joomla#4:

Fix not being able to change the type of a subfield.

This is the commit message joomla#5:

Implement a small in-memory renderCache for subform custom fields

This is the commit message joomla#6:

Prepare to load the subfields into a subform custom field directly via their own xml definition.

This is the commit message joomla#7:

After merge with PR joomla#17552 we can now successfully show the subforms options for the different types.

This is the commit message joomla#8:

The subform custom field type doesnt need a default value.

This is the commit message joomla#9:

Implement an option for a subform customfield to disable rendering the field values.

This is the commit message joomla#10:

Remove unused code and set hidden fields to not-required.

continga pushed a commit to continga/joomla-cms that referenced this pull request Sep 24, 2018

Rene Pasing
This is a combination of 10 commits. This is the 1st commit message:
First commit which integrates the subform custom field type.

This is the commit message joomla#2:

Rename manifest file for plg_fields_subform

This is the commit message joomla#3:

plg_fields_subform: Improve code comments and made the subform field type optionally non-repeatable.

This is the commit message joomla#4:

Fix not being able to change the type of a subfield.

This is the commit message joomla#5:

Implement a small in-memory renderCache for subform custom fields

This is the commit message joomla#6:

Prepare to load the subfields into a subform custom field directly via their own xml definition.

This is the commit message joomla#7:

After merge with PR joomla#17552 we can now successfully show the subforms options for the different types.

This is the commit message joomla#8:

The subform custom field type doesnt need a default value.

This is the commit message joomla#9:

Implement an option for a subform customfield to disable rendering the field values.

This is the commit message joomla#10:

Remove unused code and set hidden fields to not-required.

Signed-off-by: Rene Pasing <r.pasing@continga.de>
@dawe78

This comment has been minimized.

Copy link

commented Oct 1, 2018

First of all, thanks for developing, testing and adding nested subforms to beta3!!!

I updated to 3.9.0 beta3 on saturday and startet testing my component using nested subform, but I got an issue...

Steps:

  1. First subform name "order_articles"
  2. inside subform order_articles, nested subform "order_addons"

Click add-row for new article: row created width id "jform_order_articles__order_articles0__[nameofFields]"
Click add-row for new addon inside article: row created "jform_order_articles__order_articles1__order_addons__order_addons0__addon"

As you can see, nested subform count up article row num; subform should be jform_order_articles__order_articles0_..., but it creates jform_order_articles__order_articles1_...

I tested this behaviour only with my own component form until now, but I will do this test with a blank test form as soon as possible and report the results. Maybe someone has the same issue?

@dawe78

This comment has been minimized.

Copy link

commented Oct 1, 2018

Okay, tested on blank form, same issue... Nested subform starts counting on parent subform row num, eg row "order_articles2" subform starts at "order_articles3__order_addons__order_addons0".

@dawe78

This comment has been minimized.

Copy link

commented Oct 1, 2018

Additional information: after saving form (only parent subform rows saved) adding nested subform rows results in correct row count. So this issue occurs only on new added subform rows, after saving subform row adding nested subform rows works fine.

@zero-24

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2018

Can you please open an new issue mention this here? As this is a closed / merged issue.

@dawe78

This comment has been minimized.

Copy link

commented Oct 1, 2018

Okay, sorry...

continga pushed a commit to continga/joomla-cms that referenced this pull request Oct 1, 2018

Rene Pasing
This is a combination of 10 commits. This is the 1st commit message:
First commit which integrates the subform custom field type.

This is the commit message joomla#2:

Rename manifest file for plg_fields_subform

This is the commit message joomla#3:

plg_fields_subform: Improve code comments and made the subform field type optionally non-repeatable.

This is the commit message joomla#4:

Fix not being able to change the type of a subfield.

This is the commit message joomla#5:

Implement a small in-memory renderCache for subform custom fields

This is the commit message joomla#6:

Prepare to load the subfields into a subform custom field directly via their own xml definition.

This is the commit message joomla#7:

After merge with PR joomla#17552 we can now successfully show the subforms options for the different types.

This is the commit message joomla#8:

The subform custom field type doesnt need a default value.

This is the commit message joomla#9:

Implement an option for a subform customfield to disable rendering the field values.

This is the commit message joomla#10:

Remove unused code and set hidden fields to not-required.

Signed-off-by: Rene Pasing <r.pasing@continga.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.