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

[5.1] Fix custom field attribute 'showon' having no effect in the frontend (Take 2) #41693

Draft
wants to merge 16 commits into
base: 5.1-dev
Choose a base branch
from

Conversation

obuisard
Copy link
Contributor

@obuisard obuisard commented Sep 9, 2023

Pull Request for Issue #40762 and follow up of #39051.

Summary of Changes

The use of the parameter 'showon' has been introduced in Joomla 4.3 for custom fields.
However, a major issue is that field values are saved, even when hidden, which leads to values that are wrongly stored in the database and resulting to values that are rendered incorrectly in the front end.

It is a redo of PR #41104

Testing Instructions

In the 'Content' context:
Create a field of type 'text' named 'Conditional Text'.
Create a field of type 'list' named 'Box'. Add options (value1, value2 and value3).
Create a field of type 'list' named 'Square'. Add options (value1, value2 and value3).

Contrary to PR #41104, you now need to test the fields on 'save' when in an article.
No matter the combination tested with showon, always add text to 'Conditional Text'.
For each combination:

  • use a combination that shows the 'Conditional Text' field. Fill the value and 'save'. All values should be there after saving.
  • use a combination that hides the 'Conditional Text' field (but fill the field anyway, like if you did, as user, decide and change your mind on the values selected). Save. Try a combination that would show the text field. The value of 'Conditional Text' should be empty.

Note: all combinations have already been tested in PR #41104 and the code for matching fields has not changed, so just pick up a single combination for the tests, no need to try them all anymore.

Values of the 'showon' attribute you can try out, as examples:
'Conditional Text' will show if:
box:value1 => Box has value1
box!:value1 => Box does not have value1
box!: => Box has a value that is not empty (in this case, you could add an empty option to that 'Box' field)
box:value1[OR]square:value1 => Box has value1 or Square has value1
box:value1[AND]square:value1 => Box and Square must have value1
box:value1[OR]square!:value1 => Box has value1 or Square does not have value1
box:value1[AND]square!:value1 => Box has value1 and Square has a value other than value1
box:value1[AND]square:value1,value2 => Box has value1 and Square has value1 or value2
box:value1[AND]square!:value1,value2 => Box has value1 and Square has value3

Note: this PR does not validate the 'showon' attribute field, nor handles the issue where a hidden field has been marked as 'required'. The documentation should properly explain that a field that can be hidden should never been required.

Test wrong entries in the 'showon' parameter with values such as tada, box::, box:!, box:3:21, [AND]box:value3[OR]tada:2:3...
The combinations should be ignored and no error should be shown on the form.

To test subforms, make 'Conditional Text', 'Box' and 'Square' part of a subform field.
Note that in subforms, the 'showon' parameter must use a different syntax, since the field names, in a subform, show as 'field[id]'. For instance, if 'Box' has id 15, the name of 'box', in the subform, will be 'field15'.
So you can try testing with 'showon' values such as field15:value1.
Test the values while saving an article, either in back or frontend.

To test custom fields sent through emails, you will need to create fields in the 'Contact' - 'Mail ' context.
Create a field of type 'text' named 'Conditional Subject'.
Create a field of type 'list' named 'Sub'. Add options (value1, value2 and value3).
Make sure both fields have proper permissions to be editable in the contact form.
Set a 'showon' value for the text field, for example: sub:value1.
Now, when you go to the front-end and display the form for a contact, you should be able to switch between values of 'Sub' and check that the text field shows or not. When the email is sent, the content of that email should reflect the choices made.

Actual result BEFORE applying this Pull Request

All fields show in the front-end, no matter the values that have been set in the 'showon' parameter.
Same goes for fields that will show in emails sent through the contact component.

Expected result AFTER applying this Pull Request

Fields are not saved if hidden and do not populate the database.
The values show properly in the frontend.

Notes to developers:

  • why add the code to match the showon parameter in FieldsHelper? So it can be made available globally and used in overrides and third-party extensions easily.

  • I created onCustomFieldsBeforeSave for custom fields, an event that did not exist in the core before.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

In save, check that values are hidden. If so, clean the database if previously recorded, or do not save anything
Add showon mechanism to check if a custom field should appear or not
Hide values from subform
Prevent error if showon is malformed
Added onCustomFieldsBeforeSave to onContentAfterSave
Added onCustomFieldsBeforeSave that allows the raw value to be modified before save, allowing showon attribute configurations.
Add showon test for custom fields that are meant to be sent through email.
Added spaces.
Removed spaces
Typo

Co-authored-by: Brian Teeman <brian@teeman.net>
@obuisard obuisard marked this pull request as ready for review September 11, 2023 13:40
@HLeithner
Copy link
Member

I would like to see a test here before I merge it.

Forgotten Factory
@obuisard obuisard added the bug label Sep 12, 2023
@Fedik
Copy link
Member

Fedik commented Sep 13, 2023

in my test the "hidden field" get empty value only after second save.

I have 2 fields:
test-list: 1, 2, 3
test-text: with showon test-list:1

When I set list value to 1, add some text to the text field, and save, all works as usual.
When I change list to 2 or 3, after first save the text value still there, after second save the value is empty.

or I missing something?

@obuisard
Copy link
Contributor Author

in my test the "hidden field" get empty value only after second save.

I have 2 fields: test-list: 1, 2, 3 test-text: with showon test-list:1

When I set list value to 1, add some text to the text field, and save, all works as usual. When I change list to 2 or 3, after first save the text value still there, after second save the value is empty.

or I missing something?

I have seen this behavior, but only when I am on an article for testing, for instance, then apply the fix, then keep saving the article (all without closing the article). Once the article is closed and reopened, the behavior disappears from that point on.

@Fedik
Copy link
Member

Fedik commented Sep 13, 2023

(all without closing the article).

Yeah, it was "Apply" button

@Fedik
Copy link
Member

Fedik commented Sep 13, 2023

I have tested this item ✅ successfully on 36b31fd

In general it is working.


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

@RickR2H
Copy link
Member

RickR2H commented Sep 17, 2023

@obuisard As mentioned above I noticed that on the frontend the article has to be saved two times for the changes to take effect. After some more testing I noticed the same problem in the backend. I added a animated GIF with the behavior. Hope you can fix this! I've had some customers complaining about the field values not hiding so I was writing a javascript to unset the fields. This is a much better approach and a needed feature to boost the UX of the conditional fields.

custom-fields

@obuisard
Copy link
Contributor Author

@obuisard As mentioned above I noticed that on the frontend the article has to be saved two times for the changes to take effect. After some more testing I noticed the same problem in the backend. I added a animated GIF with the behavior. Hope you can fix this! I've had some customers complaining about the field values not hiding so I was writing a javascript to unset the fields. This is a much better approach and a needed feature to boost the UX of the conditional fields.

Thanks for the feedback Rick @RickR2H!

Yes, this is the way to go, catch showon on save and not at the output level.
I have spent days testing the PR, made presentations and not seen the behavior you described, only just after applying the patch on data that was improperly saved prior to the patch's use. So to me, it seemed to be an effect coming from the fact that the values were improperly saved before.

Do you have the same issue when creating new fields, using showon?

@AndySDH
Copy link
Contributor

AndySDH commented Sep 18, 2023

After applying the patch, I can't save an article anymore. I get this error:
json_decode(): Argument #1 ($json) must be of type string, array given /plugins/fields/subform/src/Extension/Subform.php:335

This happens when having a List value filled with multiple values. I have no idea why Subform.php would care about a List field, but apparently it does. It seems like Subform.php gets executed for every field type, for some weird reason.


On all other cases, I can't reproduce the issue reported by @RickR2H. All works on the first save for me on both backend and frontend. With previously existing fields and new fields, no difference.

@obuisard
Copy link
Contributor Author

obuisard commented Sep 18, 2023

After applying the patch, I can't save an article anymore. I get this error:
json_decode(): Argument #1 ($json) must be of type string, array given /plugins/fields/subform/src/Extension/Subform.php:335

This happens when having a List value filled with multiple values. I have no idea why Subform.php would care about a List field, but apparently it does. It seems like Subform.php gets executed for every field type, for some weird reason.


I think i understand what is going on, i need to add a test in the save event created for subform that ensures we are not in any other field

@obuisard
Copy link
Contributor Author

obuisard commented Sep 18, 2023

Sure, although why does Subform.php gets executed in the first place when saving other field types? Seems weird.

A new event is called on 'save' therefore it is triggered when each field is saved.
I have fixed the issue, can you please test again @AndySDH? Thank you!

@AndySDH
Copy link
Contributor

AndySDH commented Sep 18, 2023

I have tested this item ✅ successfully on fa573ca


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

@AndySDH
Copy link
Contributor

AndySDH commented Sep 18, 2023

All works nicely now, confirm :)

PS: Test for test? Can you help me test this? :)
#37320

@obuisard
Copy link
Contributor Author

PS: Test for test? Can you help me test this? :) #37320

Thanks for testing. I will take a look at #37320 later today :-)

explode name/value on the first : only
@RickR2H
Copy link
Member

RickR2H commented Sep 27, 2023

I have tested this item ✅ successfully on af12275

I create the fields as described in the description. Now I had to save twice to get the values stored in the DB. I had the demo content installed. I removed the previous created fields en deleted them from trash. Upon creating the fields again everything seems to work nice!
@obuisard Will the problem with double save not be present with other earlier created fields on update to J5? Or is the demo content author field the problem?


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

@HLeithner HLeithner added Feature and removed bug labels Oct 5, 2023
@HLeithner HLeithner changed the title [5.0] Fix custom field attribute 'showon' having no effect in the frontend (Take 2) [5.1] Fix custom field attribute 'showon' having no effect in the frontend (Take 2) Oct 5, 2023
@HLeithner HLeithner changed the base branch from 5.0-dev to 5.1-dev October 5, 2023 10:57
@HLeithner HLeithner added the RMDQ ReleaseManagerDecisionQueue label Oct 5, 2023
@HLeithner HLeithner marked this pull request as draft October 5, 2023 10:58
@Razzo1987
Copy link
Contributor

What is the expected behaviour if:

  • I have a field with a list that chooses whether to show the text field.
  • initially the text field is shown and valued
  • later in the list the option to not see the text field is chosen

@obuisard
Copy link
Contributor Author

What is the expected behaviour if:

  • I have a field with a list that chooses whether to show the text field.
  • initially the text field is shown and valued
  • later in the list the option to not see the text field is chosen

Before the PR: the text field will show if there is data in it, no matter what
After the PR: the text field's value is removed from the database when the change in the list has been made and the form has been saved. Therefore the text field won't show and no residual value remains

@rinenweb
Copy link

I have a feeling that the same bug also exists in J4, since I may have just reproduced it on a Joomla 4.4.0 installation.


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

@Razzo1987 Razzo1987 added this to the Joomla! 5.1.0 milestone Nov 11, 2023
@joomla-cms-bot joomla-cms-bot removed this from the Joomla! 5.1.0 milestone Nov 15, 2023
@Razzo1987
Copy link
Contributor

Update Labels on Issue Tracker


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

@Razzo1987 Razzo1987 added RMDQ ReleaseManagerDecisionQueue PR-5.1-dev and removed PR-5.0-dev labels Nov 15, 2023
@Razzo1987 Razzo1987 added this to the Joomla! 5.1.0 milestone Nov 15, 2023
@Razzo1987 Razzo1987 removed this from the Joomla! 5.1.0 milestone Feb 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug PR-5.1-dev RMDQ ReleaseManagerDecisionQueue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants