Selector treated inconsistently between InputfieldPage and FieldtypePage for repeaters

[Toutouwai]

Short description of the issue

Take the following example selector string for selectable pages in a Page Reference field named 'pr_1':

pr_2=page.id

So the selectable pages for pr_1 consist of pages where the page being edited is selected in another Page Reference field, pr_2.

This works fine in most cases, but when pr_1 is inside a repeater the selector (and therefore the selectable pages) is evaluated differently in InputfieldPage than in FieldtypePage. When choosing pages that can appear in the inputfield, InputfieldPage treats the page portion of the selector as the page being edited (the page that contains the repeater field). But when the edited page is saved, FieldtypePage rejects the value of pr_1 because it treats page as the repeater page.

It's debatable which is more correct (page is the edited page or page is the repeater page - I'd prefer the former), but in any case there should be consistency in how the selector is treated.

Suggestion for a possible fix

FieldtypePage could convert the $page argument of sleepValue() to the edited page:

// borrowed from ProcessPageEditLink
$n = 0;
while(wireInstanceOf($page, 'RepeaterPage') && ++$n < 10) {
    /** @var RepeaterPage $page */
    $page = $page->getForPage();
}

Setup/Environment

• ProcessWire version: 3.0.88

[ryancramerdesign]

Thanks @Toutouwai. Since sleepValue() delegates it to FieldtypePage::isValidPage(), as do other parts, I'm thinking that code should go in the isValidPage() method instead. Just wanted to confirm I'm understanding correctly and this is still consistent with what you are suggesting?

/**
 * Validate that that $value is a valid Page for this field
 * 
 * @param Page $value The value to validate
 * @param Field $field The field the value is for
 * @param Page $forPage The page the value will exist on 
 * @param bool $throwException Whether to throw an exception when not valid (default=false)
 * @throws WireException
 * @return bool
 *
 */
public function isValidPage(Page $value, Field $field, Page $forPage, $throwException = false) {
  if(InputfieldPage::isValidPage($value, $field, $forPage)) {
    $valid = true;
  } else {
    $n = 0;
    while(wireInstanceOf($forPage, 'RepeaterPage') && ++$n < 10) {
      /** @var RepeaterPage $forPage */
      $forPage = $forPage->getForPage();
    }
    if($n && InputfieldPage::isValidPage($value, $field, $forPage)) {
      $valid = true; 
    } else {
      $valid = false;
      $reason = $forPage->get("_isValidPage");
      if($throwException) {
        throw new WireException("Page $value is not valid for $field->name ($reason)");
      }
    }
  }  
  return $valid;
}

[ryancramerdesign]

Note I updated something in the code above afterwards, just in case you read this via email.

[Toutouwai]

Thanks, that fixes things for my example above.

But just wondering about one thing: in your revised version of FieldtypePage::isValidPage() you pass $forPage to InputfieldPage::isValidPage() before checking if it is a repeater page. If InputfieldPage::isValidPage() returns true at this point then FieldtypePage::isValidPage() returns true and the check/conversion for repeater pages doesn't happen.

Couldn't there conceivably be cases where a repeater page is valid for InputfieldPage::isValidPage() but the root container page is not? In this situation there would still be inconsistency between what is considered a valid page when submitted from the admin UI, where $forPage is always the edited page, and when submitted via the API where $forPage is allowed to be a repeater page.

Maybe FieldtypePage::isValidPage() should check if $forPage is a repeater page and if so convert it to the root container page as the first order of business?

[ryancramerdesign]

I'm not sure I understand what you are saying fully. That first call to InputfieldPage::isValidPage() doesn't need to check if it's a repeater page because if it's valid, it's valid, regardless of what kind of page it is. Only if that check fails do we need to go back and take a closer look at the context (which is the else condition), to consider the case of repeaters and expanding the context to the owner of the repeater. The intention here was to maintain the existing behavior, but also allow for the case you brought up. If you think there's an error in that logic, and it's possible to come up with a case to demonstrate it, we can always tweak it further.

[Toutouwai]

What I'm getting at is that I don't think there should be any inconsistency between what is considered a valid page for a Page Reference field in the context of Page Edit versus in the context of the API.

I've contrived an example that puts everything on the same screen for clarity.

In the screenshot below, the selector for allowed pages for field demo_page_reference is template=blank, title=page.title. In this example, the only pages using the blank template are those shown in the PageTable field.

In the screenshot above you can see that the only allowed page for demo_page_reference in this circumstance is "Blue" - it's title matches the title of the page being edited. This is true for demo_page_reference when it is directly in the page being edited, and when it is in demo_repeater. So when page.title is used in the selector for allowed pages page always means the page being edited and is never the repeater page (if page was the repeater page, "Green" would be an allowed page and "Blue" would not).

So far so good. But with the replacement method for FieldtypePage::isValidPage() shown above, when a page is added to demo_page_reference inside demo_repeater the allowed pages is interpreted differently.

'Green' is allowed to be added to demo_page_reference via the API. But when the root container page is loaded in Page Edit again 'Green' is not visible in the inputfield because it is not an allowed page in the context of Page Edit.

[ryancramerdesign]

Thanks for the well thought out example. If I make it more strict as you are suggesting, it eliminates the possibility that a "page.property" could be used to reference the repeater page itself. Thus far it has supported that, and I'm reluctant to break existing behavior, just in case anyone is relying upon it somewhere. Repeater pages exist as page owned by another, but they also exist as independent pages in their own right. So far I can't think of a good use case for a "page.property" to reference the repeater page, but also think I've got to be careful before making a change like that. When I change stuff like this, I usually hear back from someone that depended upon it. Will think over a little more before making further changes. But unless I can come up with a case where someone might be relying upon the existing behavior, I will probably change it as you suggest.

[Toutouwai]

If I make it more strict as you are suggesting, it eliminates the possibility that a "page.property" could be used to reference the repeater page itself. Thus far it has supported that, and I'm reluctant to break existing behavior, just in case anyone is relying upon it somewhere.

If a person were doing that then they would be setting values to a Page Reference field that are impossible to edit in the PW admin (apart from the unlikely scenario where they edit the repeater page directly under the Admin branch). Because when they view the repeater in Page Edit any values they had added via the API relying on "page" being the repeater page would not be visible in the inputfield and would be wiped out when the page is saved. Because as discussed above, Page Edit always treats "page" as the root container page being edited.

But think it over certainly - no rush.

[netcarver]

@ryancramerdesign Any more thoughts on this one?

[ryancramerdesign]

@netcarver I don't want to make further changes at this point just because I think the potential for side effects is higher than the potential for this to ever arise as an issue in the wild. Toutouwai makes a good point here though, so am adding a link in that portion of code to this issue report, just in case it ever arises as an issue for anyone, I can refer back to this.

[netcarver]

@ryancramerdesign @Toutouwai Ok, adding the won't fix label. Will revisit in a week with a view to closing this issue if there has been no more feedback.

/remind me to review this issue in 1 week

[reminders]

@netcarver set a reminder for Feb 19th 2019

[Toutouwai]

I'll close this and reopen if I ever strike a situation where the remaining discrepancy matters.