Pages find selector with not equal to and or-condition gets too many results

[designconcepts]

Short description of the issue

On PW 3.0.225 using a pages find selector with != (not equal to) and | (or-condition) gets more pages than previous or maybe it gets ignored at all.

pages()->find("name!=foo|bar")

If I have three pages "Foo", "Bar" and "Test". The selector now returns all pages (Home, Foo, Bar and Test).

Expected behavior

The selector should find all pages except the excluded pages in the selector with != and | (Home and Test).

Actual behavior

The selector finds all pages including the excluded pages (Home, Foo, Bar and Test).

---

Steps to reproduce the issue

1. Install PW 3.0.225
2. Create pages
3. Use pages find selector with != (euqals not ) and | (or-condition)

Setup/Environment

Server Details

Software	Version
ProcessWire	3.0.225
PHP	8.2.0
Webserver	Apache/2.4.54 (Unix) OpenSSL/1.0.2u mod_fastcgi/mod_fastcgi-SNAP-0910052141 mod_wsgi/3.5 Python/2.7.18 mod_perl/2.0.11 Perl/v5.30.1
MySQL Server	5.7.39
MySQL Client	mysqlnd 8.2.0

Server Settings

Parameter	Value
allow_url_fopen	1
max_execution_time	30 (changeable)
max_input_nesting_level	64
max_input_time	60
max_input_vars	1000
memory_limit	256M
post_max_size	128M
upload_max_filesize	128M
xdebug
xdebug.max_nesting_level
mod_rewrite
mod_security
EXIF Support	1
FreeType	1

GD Settings

Parameter	Value
Version	bundled (2.1.0 compatible)
GIF	1
JPG	1
PNG	1
WebP	1

iMagick Settings

Parameter	Value
Version	6.9.6
GIF	1
JPG	1
PNG	1
SVG	1
PDF	1
WebP	1

Module Details

Module ClassName	Version
ProcessTracyAdminer	1.1.3
TracyDebugger	4.25.9

[ryancramerdesign]

@designconcepts I wasn't able to reproduce it with name!=foo|bar (native Page property) but was able to reproduce it with title!=Foo|Bar (custom Page field) and I've pushed a fix for that. Were you just using name as an example of a field, or are you seeing the behavior with that page property?

[designconcepts]

@ryancramerdesign I was about to write an update, but you were faster. 😄

Yes, the main issue was with the selector title!=Foo|Bar
In a blank installation I had also the issue with name!=foo|bar, but on other production websites not.
So maybe it was just this field, which I have been using mostly for navigations in the past.

Thank you for the fix, I will try it out.

[adrianbj]

@ryancramerdesign - is this related to the issues that also caused #1793 ?

These sorts of bugs make me really nervous because when using PW as the backend for an application they can completely break the application logic and in some cases you won't even now until users complain, or worse still, end up with access to content not intended for them.

I am not really one for pushing unit tests, but I am starting to think that at a bare minimum PW's selector engine needs some. Any thoughts on how we could make this a community project because I know it will be a huge amount of work.

[elabx]

Any thoughts on how we could make this a community project because I know it will be a huge amount of work.

Offtopic, but sign me up @adrianbj !

[szabeszg]

...unit tests, but I am starting to think that at a bare minimum PW's selector engine needs some.

I agree. Selectors can be complex so I struggle to see how consistent manual testing could remain feasible. Additionally, unit tests would serve as excellent illustrations of the capabilities of the "selector engine".

[netcarver]

Maybe nik's old PW core test framework needs a revisit.

[matjazpotocnik]

processwire/processwire-requests#490

[ryancramerdesign]

The field!=b|c from this issue report is an interesting case. It literally says "field is not 'b' OR field is not 'c'", and it was behaving as that, as it would in PHP as well $field != 'title' || 'body'; which always returns true, whether $field is 'title', 'body', 'sidebar', or anything else. But of course that's not useful. So in a selector, we decide to interpret the OR | instead as an AND for this specific case, so that it becomes a synonym for a properly written AND condition field!=b, field!=c ... this would be a good one to add to future tests, verifying that two produce the same result.

For awhile I was using a modified version of Nik's tests, but it became increasingly cumbersome as I added more to it, and it tested with a fixed set of known data (as most tests do). Every site is a unique combination of pages, templates, fields, fieldtypes, access control, field settings, etc., all of which can affect PageFinder/selector behavior, so I found greater value in more focused, specific tests for the situation, and on different installations. And that's what I continue to do. But obviously I don't always think of everything (like here). Given the infinite combinations of possible things to test in this part, I don't think unit tests could replace what I currently do, but they could be another tool to help out for sure.

[adrianbj]

@ryancramerdesign - so was this issue a regression, or has it always been like that? I am pretty sure it's a regression, but just want to double check.

I agree that writing tests for selectors will be challenging and likely never complete and honestly I haven't ever really used them but it seems so overwhelming to be able to test all possible scenarios, but these sorts of regressions could literally destroy the reputation of startup I work for - I know that might sound dramatic, but selecting the right content for the right people is business critical. Maybe the tests should be on my end rather than PW's, but then only my project would benefit.

[netcarver]

@ryancramerdesign Would publishing your tests be helpful for others in the community - allowing them to alter/extend as needed for their own requirements?

[ryancramerdesign]

@netcarver That's a good idea, but I'm just talking about temporary tests written for a particular change or feature that's being worked on, and they are also specific to an installation. They wouldn't be useful/portable outside of that, and often not even beyond the current snapshot of the site being tested. So I don't keep an archive of reusable tests or anything like that. But that's exactly the sort of thing we could benefit from, is having something reusable like that.

[teppokoivula]

One potential approach would be getting into the habit of bundling tests with issues / fixes, and building up test coverage that way. Once an issue (such as this one here) is fixed — or while it's being fixed — write a test case for it. Not only does said test prove that the issue was actually fixed, it should also effectively prevent regressions in the future.

As a real world example, the WP kses test class contains a test called test_wp_kses_allowed_html, with comment referring to ticket 20210, which in turn details the issue it was authored for. (Many other examples in there as well.)

Just an idea.

[teppokoivula]

Loosely related, but I remember struggling quite a bit when creating the (now largely abandoned) test suite for the Version Control module. The idea was to start from a "blank" setup, create required fields and templates, install modules, etc. and then in teardown attempts to restore it to "original state". Back then I figured that it made more sense than using a "seed database", since I could easily rerun it against many different ProcessWire versions.

~200 lines of code in setUpBeforeClass and ~100 more in tearDownAfterClass. Probably broke more than a few best practices there :)

Would be interesting to know what's the state of the art test suite for ProcessWire modules these days. I'm aware that ProcessGraphQL has a a pretty comprehensive test suite, but that's just about it.

[designconcepts]

@ryancramerdesign - so was this issue a regression, or has it always been like that? I am pretty sure it's a regression, but just want to double check.

@adrianbj - The selector from the issue worked at least until PW 3.0.210. Somewhere after that it stopped working. 😉