[#41918] - [5.0] Fix ShowOnRule validation with wrong results
- Fixed in Code Base
- 25 Sep 2023
- Medium
- Build: 5.0-dev
- # 41918
- Diff
- richard67:5.0-dev-fix-showon-field-validation-empty-check
User tests: Successful: Unsuccessful:
Pull Request for Issue #41788 .
Summary of Changes
The PR currently fixes the issue linked above, but in addition it fixes a general conceptional error with the validation rule added by PR #41460 :
- Field names can not only contain alphanumeric characters but also dashes.
- Showon can be used with any kind of field, i.e. also e.g. text or editor fields, so the field values can contain any kind of characters, including the
:or!:which separate the field name from the value in showon rules. This was completely broken with PR #41788 . This PR here fixes that.
Finally, it adds unit tests for the ShowOnRule validation rule, thanks @heelc29 for these.
Testing Instructions
- Go to Content -> Fields
- Edit an existing field or create a new one
- Go to Options tab
- Edit the "Showon Attribute" field.
- Try to set to one of the below values and save:
(empty value)
box:value1
box::
box:!
box:3:21
box:#@\[](the last one is a space)
box!:value1
box!:
box:value1[OR]square:value1
box:value1[AND]square:value1
box:value1[OR]square!:value1
box:value1[AND]square!:value1
box:value1[AND]square:value1,value2
box:value1[AND]square!:value1,value2
box:value1[AND]square!:value1,value2[OR]square!:value1
box:value1,value2[AND]square:value1
box:value1[AND]square!:value1:value2
box-test-1:value1
box-test-1!:value1
box-test-1!:
box-test-1:3:21
box:value1[AND]square-test-1!:value1:value2
box:value1[OR]square-test-1:value1
box:value1[OR]square-test-1!:value1
box
[AND]box:value3[OR]square:2:3
[AND][OR]
box@abc:value1
box_abc:value1
box!abc:value1 - Try any other valid or invalid rules which come to your mind.
In addition, check the Drone logs for the unit tests without and with this PR, or run the unit tests yourself using ./libraries/vendor/bin/phpunit --testsuite Unit, and compare the results.
Actual result BEFORE applying this Pull Request
The following rules fail validation, but they should pass:
box::
box:!
box:#@\[] (the last one is a space)
box!:
box-test-1!:
The following rules pass validation, but they should fail:
[AND]box:value3[OR]square:2:3
box@abc:value1
box_abc:value1
box!abc:value1
Expected result AFTER applying this Pull Request
The following rules are validated with success:
(empty value)
box:value1
box::
box:!
box:3:21
box:#@\[] (the last one is a space)
box!:value1
box!:
box:value1[OR]square:value1
box:value1[AND]square:value1
box:value1[OR]square!:value1
box:value1[AND]square!:value1
box:value1[AND]square:value1,value2
box:value1[AND]square!:value1,value2
box:value1[AND]square!:value1,value2[OR]square!:value1
box:value1,value2[AND]square:value1
box:value1[AND]square!:value1:value2
box-test-1:value1
box-test-1!:value1
box-test-1!:
box-test-1:3:21
box:value1[AND]square-test-1!:value1:value2
box:value1[OR]square-test-1:value1
box:value1[OR]square-test-1!:value1
The following rules fail validation:
box
[AND]box:value3[OR]square:2:3
[AND][OR]
box@abc:value1
box_abc:value1
box!abc:value1
The new unit tests run with success.
Link to documentations
Please select:
-
No documentation changes for docs.joomla.org needed
-
No documentation changes for manual.joomla.org needed
joomla-cms-bot
- | Category | ⇒ | Libraries |
| Status | New | ⇒ | Pending |
| Labels |
Added:
Release Blocker
bug
PR-5.0-dev
|
||
| Title |
|
||||||
I have tested this item ✅ successfully on fbdd6ff
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/41918.
PR is ready for tests now, I've just completed the instructions.
@richard67 I tried adding an unit test for this rule: richard67#39
Can you check please?
| Category | Libraries | ⇒ | Libraries Unit Tests |
It looks like I broke phpcs ...
| Labels |
Added:
?
|
||
Your file had Windows (CR+LF), but we require Unix (LF). I recommend using an editor which shows this, like e.g. "notepad++".
@richard67 I use VS Code ... it shows only small in the corner. I will take care to change it to LF
Your file had Windows (CR+LF), but we require Unix (LF). I recommend using an editor which shows this, like e.g. "notepad++".
@richard67 I use VS Code ... it shows only small in the corner. I will take care to change it to LF
@heelc29 Can't VS Code be configured to use a custom editor and a custom comparison (diff) tool? If you've ever used notepad++ as editor and BejondCompare as diff tool, you will not want anything else anymore.
I like Beyond Compare and use it daily at work ... its easy to compare complete folders and ZIPs ?
I have tested this item ? unsuccessfully on 5f5e78d
myfield!:
works
but
my-field!:
(with a dash in between)
fails
my-field:value1
now also fails
also:
my-custom-field-abcsdf:value1
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/41918.
@AndySDH Do you know which non alphanumeric characters are allowed in field names (besides the dash)?
It should only be dash, just like an article alias.
To be honest with you, I've been personally also "misusing" a syntax in Joomla 4 to make a conditional field depend on a specific child field of another subform by doing something like this:
my-subform-field][field212!:
...which works correctly. But I know it's a bit of a mis-used syntax. I would love for it to be supported, but if it's not I'd understand.
It should only be dash, just like an article alias.
Done. Thanks for testing. Could you test again? Thanks in advance.
To be honest with you, I've been personally also "misusing" a syntax in Joomla 4 to make a conditional field depend on a specific child field of another subform by doing something like this:
my-subform-field][field212!:...which works correctly. But I know it's a bit of a mis-used syntax. I would love for it to be supported, but if it's not I'd understand.
@AndySDH This won't work anymore, and I think we should not support that because it's invalid syntax. If that worked, it was just luck.
box:value-1
Yes...
What should also be the behavior when there are just several empty spaces in the field? Right now, it fails the test.
What should also be the behavior when there are just several empty spaces in the field? Right now, it fails the test.
@obuisard Depending on the field type, the value can be anything, right?
So e.g. a text field or editor field can also contain characters which it should not contain because we use them in showon conditions as special characters, e.g. [, ], :, !, right?
If so, these characters would have to be escaped somehow in the value, otherwise a showon could not be used, and catching this with that kind of validation rule would be hard.
So either we restrict showon to a subset of fields where that can not happen,or we revert the PR #41460 completely.
We can't really restrict showon to specific types because then we restrict the showon to core fields only.
Then all we can do is to allow all characters in the value expect of those which we need to recognize the showon rules' parts (":" and "!"). If those characters then appear, showon will be broken so or so, without this PR and with this PR and also without the validation rule at all.
Depending on the field type, the value can be anything, right?
Yes exactly...
I've created a fix now which allows anything for the field values, so only field names are restricted to alphanumeric characters and dashes, and the showon syntax must be correct, i.e. conditions are separated by [AND] or [OR], and the first : or !: after the name separates the name from the condition.
box:3:21 and box:: no longer fail, but it should be expected and accepted after all.
I have tested this item ✅ successfully on caa7577
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/41918.
I have tested this item ✅ successfully on caa7577
All going well now. Nice work.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/41918.
Just now I've finished testing instructions (actual result and expected result). Are you sure you've tested all that?
Yes, all is working as expected now, tested all combinations I could think of.
Well, the unit tests do their job, so with the 2 successful human tests I think it can be RTC.
@AndySDH Thanks for testing and pushing me into the right direction. As I am not using fields with showon so often and was not the author of that new validation rule, only tried to fix it, I was not aware that field values can be anything, so it was not really clear to me how wrong the validation rule is without this PR.
@AndySDH Thanks for testing and pushing me into the right direction. As I am not using fields with showon so often and was not the author of that new validation rule, only tried to fix it, I was not aware that field values can be anything, so it was not really clear to me how wrong the validation rule is without this PR.
You're welcome! Thanks to you actually for working on this improvement. And to @obuisard as well.
Just now I've finished testing instructions (actual result and expected result). Are you sure you've tested all that?
I have tested the values you added once I saw the new test values.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/41918.
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2023-09-25 20:54:22 |
| Closed_By | ⇒ | HLeithner | |
| Labels |
Added:
?
|
||
Thx
Also tested:
dhdhdhdhd -> rightly fails
ddd::dd -> rightly fails
ddd:;dd -> rightly fails
[AND][OR] -> rightly fails
box:value1[AND]square!:value1,value2[] -> rightly fails
box:value1[AND]square!:value1,value2[UP] -> rightly fails
box:value1[AND]square!:value1,value2[UP][OR]square!:value1 -> rightly fails
box:value1[AND]square!:value1:value2 -> rightly fails
box:value1[AND]square!:value1,value2[OR]square!:value1 -> succeeds
Looks 'pretty' solid to me