[5.0] Fix ShowOnRule validation with wrong results by richard67 · Pull Request #41918 · joomla/joomla-cms

richard67 · 2023-09-25T11:57:06Z

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 [5.0] [com_fields] Showon Attribute doesn't accept correct syntax on Joomla 5 #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

obuisard · 2023-09-25T14:59:38Z

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

obuisard · 2023-09-25T15:00:22Z

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.}

richard67 · 2023-09-25T15:00:31Z

PR is ready for tests now, I've just completed the instructions.

heelc29 · 2023-09-25T17:06:45Z

@richard67 I tried adding an unit test for this rule: richard67#39
Can you check please?

richard67 · 2023-09-25T17:16:03Z

Unit tests have been added. Thanks @heelc29 .

Without this PR: Tests: 830, Assertions: 1303, Warnings: 1, Skipped: 1.

With this PR: Tests: 845, Assertions: 1318, Warnings: 1, Skipped: 1.

Difference = the 15 new tests added here. All tests passing.

heelc29 · 2023-09-25T17:19:27Z

It looks like I broke phpcs ...

richard67 · 2023-09-25T17:21:39Z

It looks like I broke phpcs ...

@heelc29 Yes, and it seems massive. I assume it's tabs being used for indentation. Will check.

richard67 · 2023-09-25T17:26:11Z

@heelc29 The problems were the line endings. Your file had Windows (CR+LF), but we require Unix (LF). I recommend using an editor which shows this, like e.g. "notepad++".

heelc29 · 2023-09-25T17:30:14Z

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

richard67 · 2023-09-25T17:34:19Z

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.

heelc29 · 2023-09-25T17:48:18Z

I like Beyond Compare and use it daily at work ... its easy to compare complete folders and ZIPs 😀

richard67 · 2023-09-25T18:40:35Z

@obuisard Could you do a quick test again if it still works, and test with field names which contain dashes in addition? Thanks in advance.

richard67 · 2023-09-25T18:45:30Z

@AndySDH Can values also contain dashes, e.g. box:value-1?

obuisard · 2023-09-25T18:50:46Z

box:value-1

Yes...

obuisard · 2023-09-25T18:51:58Z

What should also be the behavior when there are just several empty spaces in the field? Right now, it fails the test.

richard67 · 2023-09-25T18:57:08Z

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.

obuisard · 2023-09-25T19:06:14Z

We can't really restrict showon to specific types because then we restrict the showon to core fields only.

richard67 · 2023-09-25T19:09:26Z

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.

AndySDH · 2023-09-25T19:23:46Z

Depending on the field type, the value can be anything, right?

Yes exactly...

richard67 · 2023-09-25T19:42:09Z

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.

obuisard · 2023-09-25T19:48:57Z

box:3:21 and box:: no longer fail, but it should be expected and accepted after all.

richard67 · 2023-09-25T19:50:19Z

box:3:21 no longer fails, but it should be expected

@obuisard The testing instructions haven't been updated yet. This value should NOT fail validation as a field value can be "3:21" in a text field (e.g. if it represents the time of the day).

obuisard · 2023-09-25T19:51:30Z

box:3:21 no longer fails, but it should be expected

@obuisard The testing instructions haven't been updated yet. This value should NOT fail validation as a field value can be "3:21" in a text field (e.g. if it represents the time of the day).

Yes, totally agree.

obuisard · 2023-09-25T20:00:02Z

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.}

AndySDH · 2023-09-25T20:03:15Z

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.}

richard67 · 2023-09-25T20:05:13Z

Just now I've finished testing instructions (actual result and expected result). Are you sure you've tested all that?

AndySDH · 2023-09-25T20:09:10Z

Yes, all is working as expected now, tested all combinations I could think of.

richard67 · 2023-09-25T20:09:31Z

Well, the unit tests do their job, so with the 2 successful human tests I think it can be RTC.

richard67 · 2023-09-25T20:14:40Z

@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 · 2023-09-25T20:18:31Z

@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.

obuisard · 2023-09-25T20:29:05Z

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.

alikon · 2023-09-25T20:40:27Z

RTC

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/41918.}

HLeithner · 2023-09-25T20:54:46Z

Thx

* Fix ShowOnRule failing for "not empty" rule * Fix wrong split criteria for "and" and "or" * Allow comma-separated value list * Add missing brackets around "|" expression * Add missing begin and end of string anchors * Remove useless "i" modifier as it is not needed * add unit test * Fix line ending - should be Unix not Windows * A few more unit test cases * Allow dashes in field names * Allow any kind of character in field values --------- Co-authored-by: Christian Heel <66922325+heelc29@users.noreply.github.com>

Fix ShowOnRule failing for "not empty" rule

8d78d31

joomla-cms-bot added the PR-5.0-dev label Sep 25, 2023

This was referenced Sep 25, 2023

[5.0] [com_fields] Showon Attribute doesn't accept correct syntax on Joomla 5 #41788

Closed

Add new input rule for ShowOn field #41460

Merged

richard67 added Release Blocker bug labels Sep 25, 2023

richard67 added 5 commits September 25, 2023 15:21

Fix wrong split criteria for "and" and "or"

1951aa5

Allow comma-separated value list

dd7aeab

Add missing brackets around "|" expression

c2f0692

Add missing begin and end of string anchors

c030cb2

Remove useless "i" modifier as it is not needed

fbdd6ff

richard67 changed the title ~~[5.0] [WiP] Fix ShowOnRule failing for "not empty" rule~~ [5.0] Fix ShowOnRule validation with wrong results Sep 25, 2023

richard67 marked this pull request as ready for review September 25, 2023 14:46

add unit test

cb4161f

heelc29 mentioned this pull request Sep 25, 2023

add unit test richard67/joomla-cms#39

Merged

Add unit tests

261b8e2

joomla-cms-bot added the Unit/System Tests label Sep 25, 2023

Fix line ending - should be Unix not Windows

2983f61

A few more unit test cases

5f5e78d

Allow any kind of character in field values

caa7577

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 25, 2023

HLeithner merged commit a9d106c into joomla:5.0-dev Sep 25, 2023

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Sep 25, 2023

richard67 deleted the 5.0-dev-fix-showon-field-validation-empty-check branch September 25, 2023 20:55

richard67 removed the Release Blocker label Sep 25, 2023

brianteeman mentioned this pull request Jul 22, 2024

showon attribute does not seem to behave properly when using ! #41120

Open

Uh oh!

Conversation

richard67 commented Sep 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Testing Instructions

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Link to documentations

Uh oh!

obuisard commented Sep 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

obuisard commented Sep 25, 2023

Uh oh!

richard67 commented Sep 25, 2023

Uh oh!

heelc29 commented Sep 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richard67 commented Sep 25, 2023

Uh oh!

heelc29 commented Sep 25, 2023

Uh oh!

richard67 commented Sep 25, 2023

Uh oh!

richard67 commented Sep 25, 2023

Uh oh!

heelc29 commented Sep 25, 2023

Uh oh!

richard67 commented Sep 25, 2023

Uh oh!

heelc29 commented Sep 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richard67 commented Sep 25, 2023

Uh oh!

richard67 commented Sep 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

obuisard commented Sep 25, 2023

Uh oh!

obuisard commented Sep 25, 2023

Uh oh!

richard67 commented Sep 25, 2023

Uh oh!

obuisard commented Sep 25, 2023

Uh oh!

richard67 commented Sep 25, 2023

Uh oh!

AndySDH commented Sep 25, 2023

Uh oh!

richard67 commented Sep 25, 2023

Uh oh!

obuisard commented Sep 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richard67 commented Sep 25, 2023

Uh oh!

obuisard commented Sep 25, 2023

Uh oh!

obuisard commented Sep 25, 2023

Uh oh!

AndySDH commented Sep 25, 2023

Uh oh!

richard67 commented Sep 25, 2023

Uh oh!

AndySDH commented Sep 25, 2023

Uh oh!

richard67 commented Sep 25, 2023

Uh oh!

richard67 commented Sep 25, 2023

Uh oh!

AndySDH commented Sep 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

obuisard commented Sep 25, 2023

Uh oh!

alikon commented Sep 25, 2023

Uh oh!

richard67 commented Sep 25, 2023 •

edited

Loading

obuisard commented Sep 25, 2023 •

edited

Loading

heelc29 commented Sep 25, 2023 •

edited

Loading

heelc29 commented Sep 25, 2023 •

edited

Loading

richard67 commented Sep 25, 2023 •

edited

Loading

obuisard commented Sep 25, 2023 •

edited

Loading

AndySDH commented Sep 25, 2023 •

edited

Loading