Endless recursion in Page::resetTrackChanges

[BitPoet]

Not sure if this is by design - or if I'm perhaps missing the obvious. I'm just getting my feet wet with pw (2.5.3) and have created a partial sitemap template (nothing fancy, just recursing through a partial tree). When I added a page field to my sitemap template, selected a page and tried to save, I got a memory limit exceeded error. The problem appeared only when the page picked in the page field was the same as the one being edited.

    public function resetTrackChanges($trackChanges = true) {
        parent::resetTrackChanges($trackChanges);
        foreach($this->data as $key => $value) {
            
            if(is_object($value) && $value instanceof Wire) $value->resetTrackChanges($trackChanges);
        }
        return $this;
    }

Looking at this method, I guess it's easy to see that this will run in an endless loop if one of the properties of $page is once again $page. As a quick fix, this could be cured by comparing $this and $value in the conditional, but there'd still be a possibility of building circular patterns that have the same problem. Not sure how to go about that.

Edit: Of course, InputfieldPage prevents me from even assigning the same page here at all (my fault). Still, the check in Inputfieldpage::isValidPage appears to happen too late.

[adrian]

Just a quick comment - not sure if this is exactly the same problem or not:

https://github.com/ryancramerdesign/ProcessWire/issues/663

I think there still needs to be some work done for a proper fix. If you think it is the same issue, would you mind commenting there, or if not, maybe post a new issue.

[BitPoet]

Thank you for the pointer, adrian. I've commented on the issue.

[BitPoet]

I did my first fork and pull request with git - which turned out to be far less mysterious than anticipated - and Ryan already accepted my fix. Thanks!
 

Edited December 18, 2014 by horst
@BitPoet: Hope you don't mind, I added the link to the github repo.

[adrian]

Hey guys,

I have battling with this for way too many hours now and wondering if someone might have any clue what's going on, cause my brain is no longer working

I have a very customized user template with several page reference fields that allow selection of other users. The selectors for these page fields prevent selecting the current user so it's not the same as what @BitPoet reported and fixed, but it's certainly related.

Of the the page reference fields I have, it's only one that causes the trouble, but I can't see any difference between it and the others.

Here's the export of the tester (which is fine) and the subscriber (which results in the recursion).

Spoiler

{
    "tester": {
        "id": 127,
        "type": "FieldtypePage",
        "flags": 32768,
        "name": "tester",
        "label": "Tester",
        "derefAsPage": 2,
        "inputfield": "InputfieldSelect",
        "parent_id": "",
        "labelFieldName": ".",
        "labelFieldFormat": "{first_name} {last_name}",
        "collapsed": 0,
        "showIf": "user_type=1146",
        "editLinks": 1,
        "newPageParent": "",
        "operator": "%=",
        "searchFields": "name, first_name, last_name",
        "findPagesSelector": "template=user, user_type=1147, check_access=0, include=all",
        "optionColumns": 0,
        "allowUnpub": "",
        "defaultValue": "",
        "template_id": "",
        "template_ids": "",
        "findPagesSelect": "",
        "addable": "",
        "themeOffset": "",
        "themeBorder": "",
        "themeColor": "",
        "columnWidth": 100,
        "required": "",
        "requiredIf": "",
        "newPageLink": ""
    },
    "subscriber": {
        "id": 196,
        "type": "FieldtypePage",
        "flags": 32768,
        "name": "subscriber",
        "label": "Subscriber",
        "derefAsPage": 2,
        "inputfield": "InputfieldSelect",
        "parent_id": "",
        "findPagesSelector": "template=user, user_type=1146, check_access=0, include=all",
        "labelFieldName": ".",
        "labelFieldFormat": "{first_name} {last_name}",
        "collapsed": 0,
        "showIf": "user_type=1147|1148",
        "newPageParent": "",
        "allowUnpub": "",
        "defaultValue": "",
        "template_id": "",
        "template_ids": "",
        "findPagesSelect": "",
        "addable": "",
        "themeOffset": "",
        "themeBorder": "",
        "themeColor": "",
        "columnWidth": 100,
        "required": "",
        "requiredIf": "",
        "editLinks": "",
        "newPageLink": ""
    }
}

 

 

At the moment the only way I seem to be able to prevent recursion is to add a:

if($key == 'subscriber') continue;

to the foreach loop in the resetTrackChanges() method. 

There has to be something I am missing, but right now I have no idea what

Suggestions greatly appreciated!

[Robin S]

@adrian, is there a chance that somehow the value is in fact the user being edited? You could dump the ID of $value to check.

Just a guess, but I'm wondering if the strict comparison here...

$value !== $this

...doesn't catch a user page because it is a object of classname User rather than just a plain Page.

[adrian]

6 hours ago, Robin S said:

is there a chance that somehow the value is in fact the user being edited?

No chance as far as I can tell.

I have debugged the values of $this and $value and the only thing I can ascertain is that the recursion only happens with there is a selected value for the page field (this is probably to be expected), but they never match.

This is the result from being on the user with ID 1122 which has the selected subscribed user as 1100.

            if($key == 'subscriber' && $value !== '') {
                bd($this .':'. $value);
                continue;
            }

1122:1100
1100:
1122:1100
1122:1100
1100:
1122:1100
1122:1100

If I don't "continue" in that conditional, then I get the following - note the extra occurrences of the 1100 page which as I mentioned is the page the is selected from the page I'm currently on. Note that on page 1100 the subscriber field is hidden and nothing is selected.

1122:1100
1100:
1122:1100
1100:
1122:1100
1100:
1122:1100
1100:
1122:1100
1100:
1122:1100
1100:
1122:1100
1100:
1122:1100

etc etc etc

 

Thanks if you have anymore ideas!

 

[BitPoet]

You might find out more if you print out the names ($key) of all properties of type page and their value's (page) ids. There should be some kind of repeating pattern then that leads from subscriber to tester back to subscriber, but their might be (quite) some hops in between.

[adrian]

@BitPoet - You're a legend!

I had dumped everything I could think of, including your suggestion, but this comment:

Quote

pattern then that leads from subscriber to tester back to subscriber

triggered something in my tired brain

It's because one of the other page fields points back to the initial user. I didn't think about the recursion happening from another field. The question of course now is how to prevent it happening. I feel like this is a PW bug because I think it's valid to tag a user from one as a "subscriber" and yet still tag the original as a "tester" for that user.

I feel like that foreach loop in resetTrackChanges() needs to keep track of page ids that have already been processed.

What do you think about this for a solution to propose to Ryan:

    public function resetTrackChanges($trackChanges = true) {
        parent::resetTrackChanges($trackChanges);
        $processedPages = new PageArray();
        foreach($this->data as $key => $value) {
            $processedPages->add($value);
            if(is_object($value) && $value instanceof Wire && $value !== $this && !$processedPages->has($value)) $value->resetTrackChanges($trackChanges);
        }
        return $this;
    }

PS I probably need to check that $value is an instance of Page before trying to add it

[adrian]

It's a little more complicated than that - need to make sure that $processedPages is defined outside the method so it' not reset on each recursive call. I know Ryan will have his approach to this so I'm not going to bother refining right now, but if anyone is interested, here is the new issue: https://github.com/processwire/processwire-issues/issues/572

[BitPoet]

That would only catch flat recursion though. The only way to catch deeper nesting I can think of would be to pass the starting object along. Something like this (untested):

    public function resetTrackChanges($trackChanges = true, $startingObj = null) {
        parent::resetTrackChanges($trackChanges);
        if(!$startingObj) $startingObj = $this;
        foreach($this->data as $key => $value) {
            if(is_object($value) && $value instanceof Wire && $value !== $startingObj)
                $value->resetTrackChanges($trackChanges, $startingObj);
        }
        return $this;
    }

Of course, that method signature and logic might have to be implemented by all Wire derived classes that implement their own resetTrackChanges method.

On second thought, that doesn't help either. You're spot on with keeping track of seen pages outside of recursion. On the other hand, multiple resetTrackChanges calls might happen in the same program run, so there needs to be a mechanism that cleans up the tracking list.

[adrian]

3 minutes ago, BitPoet said:

Something like this

Thanks - I have referenced your idea in the Github issue - will see what Ryan says.

Cheers for the help!

 

[BitPoet]

No need for thanks. I hit my head on that problem a few times myself, so it's pure self-preservation in the long run My approach above, as I figured out just a little too late, won't work out anyway, since it might not be original object that starts the recursion.

[Robin S]

@adrian, until a fix is found a workaround is to set one (or both) of the Page Reference fields to dereference as a PageArray.
Related issue: https://github.com/processwire/processwire-issues/issues/152

[adrian]

6 hours ago, Robin S said:

@adrian, until a fix is found a workaround is to set one (or both) of the Page Reference fields to dereference as a PageArray.
Related issue: https://github.com/processwire/processwire-issues/issues/152

Thanks for pointing this out - I'd forgotten about that issue. Unfortunately I have a lot of code and a lot of Page Reference fields that rely on single page / null so I am hesitant to change at this point in the project. I am testing your other solution though and so far so good