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

Selector for field in nested repeater item wrongly matches pages where top-level repeater is empty #205

Closed
Toutouwai opened this issue Mar 1, 2017 · 9 comments

Comments

@Toutouwai
Copy link

Short description of the issue

A selector for a field contained in a nested repeater (a repeater inside another repeater) wrongly matches pages where the top-level repeater field is empty.

$results = $pages->find("my_repeater.my_nested_repeater.my_textarea%=some-string-that-matches-nothing");

Steps to reproduce the issue

  1. Create textarea field 'my_textarea'
  2. Create repeater field 'my_nested_repeater' and add my_textarea as a subfield
  3. Create repeater field 'my_repeater' and add my_nested_repeater as a subfield
  4. Add my_repeater to a new template and create one or more pages using the template. Do not add any repeater items to my_repeater.
  5. Note that the following selector matches any pages using the template where my_repeater is empty:
$results = $pages->find("my_repeater.my_nested_repeater.my_textarea%=some-string-that-matches-nothing");

Setup/Environment

  • ProcessWire version: 3.0.52
  • (Optional) PHP version: 7
@somatonic
Copy link

somatonic commented Aug 14, 2018

I can confirm this (as of 3.0.98) and am baffled it's one year since this report and nothing happened.... :(

@ryancramerdesign
Copy link
Member

I've looked at this one out a couple times but haven't been able to figure it out. @somatonic your wording suggests you may know more about how to fix it than I do? If so please post more details. Otherwise, this issue will have to remain open until someone affected by the issue can assist with the debugging of it.

@jlahijani
Copy link

Video of issue:
https://www.youtube.com/watch?v=CAWDGCRfAl4

@jlahijani
Copy link

Hmm, now that I'm looking at this with a fresh pair of eyes, if I simply added my_repeater.count>0 to my query in the video, that would have resolved it.

@Toutouwai
Copy link
Author

Another quirk of this issue: if you use "multi dot" syntax to attempt to match a field that is not even in the repeater the selector will match pages where the repeater is empty. So if field my_repeater does not contain the images field then this selector will match pages where my_repeater is empty.

$results = $pages->find("my_repeater.images.description=foo");

if I simply added my_repeater.count>0 to my query in the video, that would have resolved it.

That selector clause shouldn't be necessary since x.y.z=value by implication cannot (and should not) match a page where x is empty.

@Toutouwai
Copy link
Author

I took a stab at debugging this issue and the problematic part seems to be this section of PageFinder.

This section comes into play because the count the array of matching page IDs is zero (no pages match the subselector), and the result of this section in the case of my example above is my_repeater.id=0. This selector will match any pages where my_repeater is empty, which is obviously not what is wanted in the case of a subselector failing to match any pages.

I don't grasp everything that this section of code is doing, but it contains this comment...

// subselector resulted in 0 matches
// force non-match for this subselector by populating 'id' subfield to field name(s)

...which sounds like the intention is to force a non-match. But if that's the case couldn't the non-match be forced directly by using $selector->forceMatch = false?

@Toutouwai
Copy link
Author

Toutouwai commented Mar 5, 2019

More investigation...

A selector page_reference_field.id=0 will not match pages where page_reference_field is empty, yet a selector repeater_field.id=0 will match pages where repeater_field is empty. This seems to point to a problem with FieldtypeRepeater::getMatchQuery().

This part seems to be saying a search for repeater_field.id=0 or repeater_field.id='' should be treated the same as a search for repeater_field.count=0, but that doesn't sound correct.

ryancramerdesign added a commit to processwire/processwire that referenced this issue Mar 5, 2019
@ryancramerdesign
Copy link
Member

@jlahijani Thanks for the video, that was helpful to see.

@Toutouwai I think you tracked it down! Nice job—thanks. It was that FieldtypeRepeater::getMatchQuery() that was converting an id=0 to a field.count=0, which isn't what we want here. The id=0 should instead force a non-match. I've pushed an update that corrects that. I think this fixes it except that I'm not 100% confident because I was never able to duplicate the "another quirk" you mentioned, that was always returning a no-match for me.

@Toutouwai
Copy link
Author

Great, thanks @ryancramerdesign, that fixes all the cases I tested with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants