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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'subfields' custom field type #22446

Open
wants to merge 42 commits into
base: 3.10-dev
from

Conversation

Projects
None yet
@continga
Copy link
Contributor

continga commented Oct 1, 2018

Summary of Changes

This pull request adds the possibility to create custom fields of a new type subfields.

Before this PR, it was only possible to add "basic" types as custom fields to articles, contacts etc, whereas "basic" types would be "text", "media", "integer", etc.

This PR adds the possibility to add a subfields custom field to an article, contact etc., whereby the idea behind subfields is that a user can create a list of sub fields that his custom field should contain, and those sub fields will then be presented to the user in the backend as a possibly repeatable list.

Testing Instructions

Install the patch, go in the backend to e.g. Content -> Fields -> New, select Type=Subfields, set Title=test123, under Fields add a simple Text field with name and label test1, and another simple Text field with name and label test2. Save & close the custom field.

Go to Content -> Articles -> New, switch to tab Fields, and check whether the new subfields custom field is working as it should (so displaying a repeatable version of the configured fields). Insert an Article Title and something in the custom field, and save & close the article. Afterwards go to the frontend and check whether the article and its content is being displayed correctly.

Expected result

Custom field type subfields can be edited correctly, its content can be added correctly in the backend when editing/creating an article/contact/etc, and the custom field values will be correctly displayed in the frontend.

Documentation Changes Required

I have written a small documentation which mainly targets developers and describes the needed actions to make a layout override for the rendering of the subfields value. This documentation can be enhanced by everyone interested to describe the main functionality. https://docs.joomla.org/Custom_fields_type:_Subfields

Additional information

This PR is based on #17552

I created this PR against the 3.10-dev branch, also because I would like to have enough time to work on possible feedback coming in (not against staging, but directly against 3.10). But can we all please work together so this really ends up in 3.10, and also in 4.0? I am very willing to put much work into this, but I am dependent on you guys' feedback. So please, tell me! 馃檪

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 #2:

Rename manifest file for plg_fields_subform

This is the commit message #3:

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

This is the commit message #4:

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

This is the commit message #5:

Implement a small in-memory renderCache for subform custom fields

This is the commit message #6:

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

This is the commit message #7:

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

This is the commit message #8:

The subform custom field type doesnt need a default value.

This is the commit message #9:

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

This is the commit message #10:

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

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

@continga continga requested a review from brianteeman as a code owner Oct 1, 2018

@continga continga force-pushed the continga:fields-subform-3.10 branch 2 times, most recently from 249dab9 to af81e22 Oct 1, 2018

Rene Pasing

@continga continga force-pushed the continga:fields-subform-3.10 branch from af81e22 to bcb2601 Oct 2, 2018

}
echo '</ul>';
/**

This comment has been minimized.

@Quy

Quy Oct 2, 2018

Contributor

Remove commented code?

This comment has been minimized.

@continga

continga Oct 14, 2018

Contributor

Should I really remove it? I added this on purpose, including the comment above, to help other developers, especially designers which are creating layout overrides, to create better templates

This comment has been minimized.

@mbabker

mbabker Oct 14, 2018

Member

The code is not a documentation source. This belongs in the documentation as an example or as an alternate layout, but not as a full blown commented code block.

This comment has been minimized.

@continga

continga Oct 14, 2018

Contributor

True though. By the way, how would the workflow be for creating decent documentation for this feature? Should I create a new page in the docs wiki and link it in this PR?

@laoneo

This comment has been minimized.

Copy link
Member

laoneo commented Oct 3, 2018

What is the difference to #20243? For me it looks like it has a similar functionality.

@stutteringp0et

This comment has been minimized.

Copy link
Contributor

stutteringp0et commented Oct 11, 2018

Any chance I could convince you guys to rename this? My subform field plugin has been in the JED since Apr 26 2018 and will not be installable when this is merged.

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Oct 11, 2018

@stutteringp0et The concept of adding a subform goes back much further than this pr see #17552

@mbabker

This comment has been minimized.

Copy link
Member

mbabker commented Oct 11, 2018

I hate to sound like I'm coming across as a jerk here too, but "subform" is what we call it in core and as annoying as it has to be for you it'd be equally as awkward to have a custom fields plugin in core named something completely different to avoid the naming collision with your plugin.

@stutteringp0et

This comment has been minimized.

Copy link
Contributor

stutteringp0et commented Oct 11, 2018

The second guy to cross the finish line doesn't get the first place trophy. They talked about it in #17552, the difference is that I actually did it.

core-subform maybe? I worked on mine for a long time too and it's not like I can get the JED to rename my extension, I have to re-submit. My users will probably not want to give up features to use the core version.

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Oct 11, 2018

Well that's what happens when you work on something for yourself instead of contributing to help everyone benefit - sorry you're not going to get any sympathy from me

@stutteringp0et

This comment has been minimized.

Copy link
Contributor

stutteringp0et commented Oct 11, 2018

56 extensions in the JED, more than 80% of them are free, but I'm not helping anyone.

Gotcha

@mbabker

This comment has been minimized.

Copy link
Member

mbabker commented Oct 11, 2018

The second guy to cross the finish line doesn't get the first place trophy. They talked about it in #17552, the difference is that I actually did it.

By this same logic, it could easily be argued that any feature implemented by an extension couldn't be added to core because it will conflict with or completely disable/destroy an existing extension. Yes, it sucks for those who have those extensions and come into those conflicts. No, I don't think it's appropriate to rename a core feature to some other name to deconflict with an extension that used the same name.

@stutteringp0et

This comment has been minimized.

Copy link
Contributor

stutteringp0et commented Oct 11, 2018

I hope you recognize what this attitude does to morale for us lowly 3rd party developers. This isn't the first time, it's the 5th (for me). I have a hard time being enthused about releasing any new extensions in this environment.

My comments on issue #11905 were deleted by the Joomla user, so it's not like there wasn't prior knowledge that my extension existed.

No sympathy? Yes, that's quite clear.

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Oct 11, 2018

iirc i deleted the comment because they were an advert for your extension which is not appropriate here (but my memory might be failing) however that was in aug 2018

@mbabker

This comment has been minimized.

Copy link
Member

mbabker commented Oct 11, 2018

Well, I don't have those email notifications anymore, but if the comment was perceived as an advertisement for an extension then it would be rightfully deleted.

"Subform" is the form field type in core, therefore a plugin adding support for it to custom fields should carry the same name. We should not have to prefix everything with "core-foo" or "joomla-foo". Again, yes, it totally sucks if you are the person who put out an extension with that exact name. But I don't think it's worthy of us renaming things in core to create even more confusion (if someone's on a site with your "Fields - Subform" plugin and someone updates core to get a new "Fields - Core Subform" plugin, how is that not confusing to users?) or making everything in core follow a plg_fields_core_text naming convention.

@stutteringp0et

This comment has been minimized.

Copy link
Contributor

stutteringp0et commented Oct 11, 2018

My comment was deleted August 2018, but I made it sometime around March or April in reply to pepperstreet who mentioned me in a comment on 3/30 (I still have the emails).

How confusing is it going to be for a user who upgrades Joomla and finds their subforms no longer work because they've been overwritten by a new core field of the same name? That's the kind of support call I'm going to get.

I seriously doubt you would ever consider naming a core extension com_comprofiler. Beat is a big fish, and I'm just a small fish.

@AndySDH

This comment has been minimized.

Copy link
Contributor

AndySDH commented Jan 17, 2019

  1. Like @regularlabs suggested earlier, maybe in the subfields creation form you could add an edit button that opens a modal to edit the sub field properties. This is by no means necessary or vital as much as the other things mentioned, but would be a cool extra usability feature.

Idea regarding this. We could change the whole select form to be done like for Joomla article/category menu items. So instead of being a dropdown, it could be something like this:

image
(Of course with "Select a Custom Field" instead of "Select an Article")

Where it then expands a modal with a list of Fields looking exactly the same as the Field List view, along with filters on top as well. So exactly like a menu item for an article is created. This would also solve issue n.2, as the field types would be displayed along the field names in the field list.

And you also have the "Create" button there to create a new one.

After inserting it, the form would turn into this:

image
(Again of course with "Select a Custom Field" instead of "Select an Article")

With the ability to edit the field or clear it to select another one.

@AndySDH

This comment has been minimized.

Copy link
Contributor

AndySDH commented Jan 17, 2019

  1. First thing first, we need to implement a way for a custom field to optionally be made available only to be used inside subfields. There is a big chance that you might want to have certain fields to be ONLY used inside subfields and not be displayed normally.

Regarding this, probably the easiest way to do this (without adding any extra option), is the ability to leave the category assignment empty:

image

However, right now, when leaving that empty and saving, it will automatically be filled with "All".

image

Simply disabling this auto-fill behavior - thus allowing to assign a field to 0 categories - would make that field not visible in any article form normally. (therefore making it possible to only use it as a sub field)

@AndySDH

This comment has been minimized.

Copy link
Contributor

AndySDH commented Jan 17, 2019

  1. Another issue

This seems to work only with PHP 7.2 right now.

With PHP 5.6, 7.0 and 7.1, when you try to create a subfields field you get:
Fatal error: Call to undefined method DOMNodeList::count() in plugins/fields/subfields/subfields.php on line 72

Rene Pasing added some commits Jan 17, 2019

Rene Pasing
Change the rendering of subfields instances, it will now correctly pa鈥
鈥s sub field instances and render them separately and combine the result
Rene Pasing
@continga

This comment has been minimized.

Copy link
Contributor

continga commented Jan 17, 2019

Hi all, I just pushed an update.

@Quy thanks, fixed.

@AndySDH & @regularlabs thanks for your feedback. Here my response:

  1. Good idea, but how should we do it exactly? Just adding a checkbox in the "Options" tab seems a bit overkill, not sure whether this really is good UX. Leaving the categories empty like suggested by you is also kind of counter-intuitive I think. Maybe somebody has a better idea?

  2. Done.

  3. Done, will now be deduplicated when the subfields is being saved.

  4. This is the default behaviour of the subform field type. Not sure whether we really want to change that. That means every time we edit an article, an "empty row" will get added when there were no values, and we get an empty row saved then too, if not removed manually. I don't think this is a good idea?

  5. Not fixed yet. I have to think about it.

  6. This is already the case.

  7. Done.

  8. This is already the case.

  9. This is again the default behaviour of the subform field type. Not sure whether we can fix this. I need to think about it.

  10. Tbh, this is really a nice idea, especially doing it like the "Select an article" thing, but I don't think I will implement that in this PR. It is not vital, it is not important, it is something nice-to-have. We can maybe work on that when this PR finally got merged sometime, and then work on that as a follow-up PR.

  11. The repeatable type should not be existant at all when using this codebase, hence also not appear there. Also, see next paragraph.

  12. Fixed.

Regarding how to continue with the repeatable plugin: Can one of you guys maybe implement it like discussed in a fork, and I will merge it back? Or, maybe we should even create a new issue to handle this. I think it would be a good idea to split these two things. Then I could just undo my changes to the repeatable plugin here and we handle it in another issue. A PR for that issue can then base on the code from this PR. This will also allow us the split the discussion, as this starts to get a bit messy.

TL;DR All fixed but point 1, 5 and 9.

@AndySDH

This comment has been minimized.

Copy link
Contributor

AndySDH commented Jan 17, 2019

Wow, @continga, great work! Didn't expect it to be addressed all so quickly :)

  1. I agree, I'm not entirely convinced about the empty category method myself either. At the same time, I'm thinking that if a field is set to be used only as child field, then the category selection doesn't make sense for it anymore. Maybe it could be be something like...

image

It's set to "No" as default. If you set it to "Yes", the Category input disappears (hides). And if it's set to Yes, it won't display in any form in any category, but will only be available to be inserted inside a subfields. What do you think?

2, 3, 7, 12. Awesome!!! Looks great. Well done :)

  1. Uuh I see what you mean... I understand. Yeah, not ideal to save empty rows. Unless there is a way to check whether all values are empty, and if they are, to not store the row. Is it possible somehow?

  2. As @regularlabs suggested, I think a good idea is to get rid of the 'required' option altogether, and make the subfields rely on the inherited 'required' options of the child fields. What do you think?

  3. Yep, noticed that later!

  1. I'm afraid that's not already the case, currently the field values are stored in the database with the field names and not with the field IDs. See:

cattura

As you can see, both the parent field names and child field names are used in the database value storing. For the child items it's best to use the ID, while as far as the parent, we don't need an identifier at all as we're already inside a value of the correct field_ID column, so I think each row could be simply stored as generic row0, row1, row2, and so on for all fields. (without the name of the parent). As the names of the fields (both parent and child) can always be changed and this would cause issues.

  1. I see. However it's pretty bad usability if you can't change a field from repeatable to non-repetable (or viceversa) without breaking it and losing all of the values. So I think it should be addressd.

  2. Sure, that sounds good 馃憤

  3. Right, but in cases that some users still have it installed for whatever reason... Even if they do, it should not be presented to them as an available sub field, so I guess you should exclude it from the selectable fields? Or it's not worth it?


  1. [new] Another thing I noticed:

If a field have no values filled in the form in any of the cells, the field is still displayed in the frontend like:
, , ,
(just commas)
This also happens when the field is set to NOT be repeatable.
Do you have a way to completely prevent the display of the field if all values are empty?

Thanks again for your great work and effort!

@AndySDH

This comment has been minimized.

Copy link
Contributor

AndySDH commented Jan 17, 2019

Still a PHP issue now in 5.6 only: Parse error: syntax error, unexpected '->' (T_OBJECT_OPERATOR) in /plugins/fields/subfields/subfields.php on line 79

@regularlabs

This comment has been minimized.

Copy link
Contributor

regularlabs commented Jan 17, 2019

  1. You could force-add the empty row if any of the sub fields are required.
    If not required, then you need to manually add the row...
@AndySDH

This comment has been minimized.

Copy link
Contributor

AndySDH commented Jan 17, 2019

Another alternate to n.1 would be adding a "None" option to Categories:

administrator/components/com_fields/models/forms/field.xml
1547756561

And should probably also check when saving that if there is a -1 value in the cat list, that it removes any others. As you don鈥檛 want to be able to save it to None + others:

administrator/components/com_fields/models/field.php
1547756516

PS: @regularlabs might have had something to do with this idea and might have sent me the screenshots. But that's just a rumor.

Up to you which one you prefer between this and the toggle in earlier post :)

Rene Pasing added some commits Jan 19, 2019

Rene Pasing
Change the 'required' flag of the subfields type and the rendered sub鈥
鈥orm in the backend:

- Hide the "required" radio selection when creating a subfields instance
- The "required" attribute of the subform in the backend is now decided by its sub fields
- Set the 'min' attribute to 1 if the subform should be required
Rene Pasing
Allow the fielname to be overriden when using a subform form field.
The fieldname of a subform is being used to construct the prefix of the array keys for
the subform rows. See e.g. layout joomla.form.field.subform.repeatable-table.section
where $basegroup is being passed as data-base-name, which comes from the layout
joomla.form.field.subform.repeatable-table, where $fieldname is being passed as value.
This $fieldname is coming $displayData, which comes from JFormFieldSubform::getInput(),
which takes $this->fieldname for $data['fieldname']. So we need to be able to override
the JFormField::fieldname attribute on a subform, which by default is not possible,
see also JFormField::setup() where it is not possible to set the fieldname. So we do this
in the subform type directly.
Rene Pasing
Change the form (XML) layout of the subfields field when adding data 鈥
鈥n the backend.

This makes sure that data given is not being saved in the DB #__fields_values using
the sub fields name (which can change), but their custom field id. Also this makes sure
that the array index of the subform is 'row' and not the name of the subfields instance.
Rene Pasing
@continga

This comment has been minimized.

Copy link
Contributor

continga commented Jan 19, 2019

Hoi, I just pushed another update. Following things have been done:

4: Changed so that when at least one sub field is required, always one row will be shown (like discussed).

5: The 'required' option is now not available for the subfields custom field, and its value will be taken from the child fields, like discussed.

8: Ah, I thought you meant how the relation parentfield<->subfields is being saved in the database, not the values themselfes for items. However, should be fixed now!

9: I don't think we should do this in this PR. This would mean changing a lot of logic for the subform type, and I don't know how this would affect existing setups. So let us leave this like it is.

13: Fixed.

The additional php5.6-compatibility thing has been solved too.

Still TODO: Setting fields as available-for-subfields-only (maybe with the category-solution? I have not decided yet) and point 9 (maybe we should really just hide the repeatable type here, and handle the removal of the repeatable type in another issue, but have not decided finally).

Is it correct, that those currently are the only two missing points? I must admit I kind of lost overview... All those comments with feedback, it is getting a bit confusing... :-) But thanks guys, seems like we are finally getting there! :D

@Fedik

This comment has been minimized.

Copy link
Contributor

Fedik commented Jan 19, 2019

  1. Right now you have to click on the "+" to make the first row even appear . The first row should already be there.

This is correct behavior. That why you have a "+".
What if you do not need any row/value?
There should not be any Row if min attribute are not defined or less than 1

  1. Changed so that when at least one sub field is required, always one row will be shown (like discussed).

This is wrong.
There 2 different cases: Subform Field required, and One of the children field are required.
They both are valid, and should not be mixed.
Subform Field required: means that the subform should have at least one row (mainly useful for multiple)
One of the children field are required: means , IF there at least one row, then the required field should have a value.

  1. Bug

This not a bug.
This is correct behavior of the subform field and should not be changed. Important point, that the 'multiple' attribute should not be changed if field already has a stored value, because it will destroy your data.

@AndySDH

This comment has been minimized.

Copy link
Contributor

AndySDH commented Jan 19, 2019

@continga Nice! Will be testing your latest additions soon, and yep I confirm those are the only points left to be addressed :)

  1. One of the children field are required: means , IF there at least one row, then the required field should have a value.

@Fedik I disagree. If one of the children is required, it means it has to have at least one value in one row. So one row should become inherently required as well. Which is what @continga implemented now. However, if you guys feel like your idea is better, I'm also okay with it. It's no big deal to change this to the way you proposed, makes little difference. So no problem, trivial stuff.

  1. This not a bug.
    This is correct behavior of the subform field and should not be changed. Important point, that the 'multiple' attribute should not be changed if field already has a stored value, because it will destroy your data.

@Fedik Well, it might not be a "bug", but if it's not a bug, it's a very bad design choice. However, as mentioned, it can be addressed in a different PR if anything.

@AndySDH

This comment has been minimized.

Copy link
Contributor

AndySDH commented Jan 19, 2019

A couple of more things that I noticed:

  • When the field is set to be repeatable, it looks like that when trying to save an article without compiling a required sub field, the page will generate an error at the top but without stating what the error is (empty error). It should be saying:
    "Invalid field: [name of the field]"

  • Issue with a calendar sub field: First filling (and saving) and then emptying a calendar sub field, makes it still store in the database value as "0000-00-00 00:00:00", and outputs it as "-0001-11-30". Instead if should obviously not be stored when empty.

Everything else that you did today @continga seems to work as expected, fantastic job man!
Great stuff with the triple checks of empty values, empty rows and empty field altogether in the template file.
We're indeed getting there! :)

@AndySDH

This comment has been minimized.

Copy link
Contributor

AndySDH commented Jan 19, 2019

I found a super weird issue. Here's how to reproduce:

  • Create a list field (to be used as sub field), let's call it "Fruits" As options, add an option with a name "Apple" (on the left) and a option value of "1" (on the right)
  • Create another list field (to be used as sub field), let's call it "Animals". As options, add an option with a name "Cat" (on the left) and also an option value of "1" (on the right)
  • Create two separate subfields field types. One having "Fruits" field as its only sub field. And the other having "Animals" as its only sub field, - Edit: This also happens if both "Fruits" and "Animals" are sub field of the same subfields.
  • Now go in an Article Form. In the first subfield, fill-in the value "Apple". In the second subfield, fill in the value "Cat".
  • Visit the article on the frontend. Both fields will show value "Apple".

TL;DR: If the option value is the same for two field values in two different fields (in this example "1" for both), the value of the first field will show in both fields.

@Fedik

This comment has been minimized.

Copy link
Contributor

Fedik commented Jan 20, 2019

If one of the children is required, it means it has to have at least one value in one row. So one row should become inherently required as well.

Nope, it doesn't meant this.
In your app you can be no need a value from the subform field, but if it exist then one of its field must have a value. How you deal with that, if you always force to "required"?
As I said it is different cases, and should not be forced.

That very easy:
if you need at least one Row in the subform, then you set it to required="true" (optionally can add min=1);
if you need some Text in the subform to be filled, then you set required="true" to the text field;
if you need at least one row with some text to be filled, then you set both to required="true" ;

Well, it might not be a "bug", but if it's not a bug, it's a very bad design choice

It is correct design choice.
The field (not only subform but any other field) should not care about data structure, the field must receive the value already in correct structure.
The structure of the data should be handled by a app model.

@AndySDH

This comment has been minimized.

Copy link
Contributor

AndySDH commented Jan 20, 2019

That very easy:
if you need at least one Row in the subform, then you set it to required="true" (optionally can add min=1);
if you need some Text in the subform to be filled, then you set required="true" to the text field;
if you need at least one row with some text to be filled, then you set both to required="true" ;

Ok, sounds okay, I have no problem with this way of doing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment