processwire / processwire-issues

ProcessWire issue reports.
44 stars 2 forks source link

Breaking change to matching empty PageAutocomplete field in inputfield dependencies #1873

Closed Toutouwai closed 7 months ago

Toutouwai commented 8 months ago

Short description of the issue

There has been a breaking change to matching an empty PageAutocomplete field in inputfield dependencies. I don't know exactly when the change occurred but the below compares results for PW 3.0.210 Master and the latest PW 2.0.335.

In PW 3.0.210 you could match an empty PageAutocomplete field named "select_page" with this show-if selector:

select_page=''

That conforms to the documentation for inputfield dependencies which says:

Matching blank or not-blank To match a blank value, specify an empty quoted string. Quotes may be double or single quotes, it does not matter.

The selector didn't work for all "single" Page Reference inputfields but it worked for three of the four "single" inputfields in the core.

select_page='' Select Radio Buttons Page List Select Page Auto Complete

Or you could think that a different part of the documentation is applicable, where it describes using "count" when matching Page Reference fields. But this was less supported in 2.0.210 and didn't work for either Page List Select or Page Auto Complete.

select_page.count=0 Select Radio Buttons Page List Select Page Auto Complete

In PW 3.0.235 the more obvious option of select_page='' doesn't work for Page Auto Complete or Page List Select, so this seems to be a breaking change. For Page Auto Complete select_page.count=0 doesn't work and the only selector I could find that does work is select_page=0 but this syntax isn't mentioned anywhere in the inputfield dependencies documentation and it's not intuitive as a selector for an empty field. It also doesn't work for two of the four core inputfields.

In PW 3.0.235 the results are:

select_page='' Select Radio Buttons Page List Select Page Auto Complete

select_page.count=0 Select Radio Buttons Page List Select Page Auto Complete

select_page=0 Select Radio Buttons Page List Select Page Auto Complete

To summarise:

  1. The same selector syntax should work for all core inputfields that can be used for a "single" Page Reference field. It would be one thing if a particular inputfield isn't supported for dependencies (the docs mention this is possible but don't name any of the inputfields in question here) but it's weird to have all the different inputfields working but requiring different selector syntax, meaning the user has to change the syntax if they change the inputfield. Based on my reading of the docs the supported syntax for an empty "single" Page Reference field should be select_page=''.
  2. The select_page='' syntax was the best working in PW 2.0.210 (3 of 4 inputfields) but is now the least working (1 of 4 inputfields) and this is a breaking change for Page Auto Complete and Page List Select.

Setup/Environment

szabeszg commented 8 months ago

My comment will not help in fixing the issue but I cannot help raising the issue of the non-existence of complete and automated tests. At least selectors should be tested, I think: https://www.lambdatest.com/blog/best-php-testing-frameworks-2021/

A breaking change in selectors is what might truly jeopardize business logic.

ryancramerdesign commented 7 months ago

@Toutouwai Thanks, I think I've got it fixed now to work as intended. Which I'll describe here.

When it comes to matching different types of selects, there's the question of whether it's a select single or a select multiple, and whether or not there is an unselected option to represent no selection. Single select elements would have an unselected option, whereas radios and checkboxes wouldn't.

The count option is for matching multi-value Inputfields or those that use multiple inputs. Checkboxes and radios are a good example. So if you wanted to match no selection on a multi-value Inputfield, then you'd do field.count=0.

The blank string is for matching single-value Inputfields that have a blank selectable option. InputfieldSelect is a good example. So you could match no selection with field= or field=''.

When you use single-page selection (InputfieldPage, InputfieldPageListSelect, etc.) it'll use a 0 rather than blank string to represent no selection, since it is now referring to page IDs, and 0 represents NullPage aka no-selection. But the intention is that you can use blank string or 0 interchangeably. I've pushed an update that normalizes that so that it treats blank string and 0 the same way for these Inputfields, as it wasn't previously. So now you should be able to match 0 or blank string.

For multi-page selection (InputfieldPageListSelectMultiple, Checkboxes, etc.) or those with multiple inputs (i.e. radios), you'd still want to use field.count as it would have to match one <input> or <option> or another, and there would be no input element to match a field='' condition.

Also note there can be difference between using something like PageAutocomplete as part of a ProcessWire page field versus using it outside of one (like in a module config or something). It looks to me like inputfields.js may be or has been ignoring things like InputfieldPageAutocomplete or InputfieldPageListSelectMultiple when used outside of a ProcessWire Page field (since the Inputfield markup would lack the InputfieldPage class), so this is something I'm still looking into.

Toutouwai commented 7 months ago

Thanks @ryancramerdesign, it's working great now for Select, Page List Select and Page Auto Complete.

I know you said that to match Radio Buttons the syntax should be field.count but I don't think a person reading the docs will expect this because it says...

"When attempting to match the value from a field containing multiple selections, you may match based on the quality of values selected by using the count subfield in your selector field."

...and a Radio Buttons field can't contain multiple selections.

For the sake of consistency with the other "single option" inputfields could we not interpret the selector field='' as being the same as field.count=0? If I add...

if(condition.value == "''") conditionSubfield = 'count';

...at the top of getCheckboxFieldAndValue() then it works as I would expect it to based on the docs.

This could be made more specific by doing some additional check to work out if the inputfield is InputfieldRadios but when I tested it worked fine with InputfieldCheckboxes too and I think it would be safe to assume that if a user enters field='' they want to match an empty inputfield regardless of the type.

ryancramerdesign commented 7 months ago

@Toutouwai That makes sense to me, good suggestion, I have added it, thanks.

Toutouwai commented 7 months ago

Thanks!