Page instance from "sorted" hook cleared from cache

[da²]

Hello,

EDIT: this original post wasn't clear, code in this post is way more explicit to show the thing I'm talking about.

 

I'm not asking how to manage this problem, but is it a bug? And if not, could you explain why this behavior?

To make it short: why is the Page instance given by HooEvent->arguments(0) not the same as the one given by find() function (children() here)? Shouldn't it be cached and the same?

 

My use case: I have a method "foobar" called by several hooks. This method process sibling pages of the one received in parameter (using $page->parent->children()), and save the other pages if modified, but does not save the one received in parameter because it can come from a saveReady hook and so the page is already being saved.

The problem comes from a Pages::sorted hook, after calling "foobar" method I save myself the page using the Page instance given by hook event ($event->arguments(0))... And so I'm not saving anything because it's not the same instance that was modified in "foobar" method.

I know how to manage this, but that looks like a cache bug, isn't it?
When "foobar" is called by a saveReady hook, changes are saved, so instance used internally by PW is same as children() one.
But when I save with hook instance ($event->arguments(0)), changes are not saved.

Here is a kind of demonstration, we can see Page instance from HookEvent is not the same as get() or find():

wire()->addHookAfter('Pages::sorted(6778)', test(...));

function test($event)
{
    $eventPage = $event->arguments(0);
    $findPage = wire()->pages->find(6778, ['getFromCache' => true])[0];
    $getPage = wire()->pages->get(6778);

    wire()->log->message('$eventPage === $findPage ' . ($eventPage === $findPage)); // False
    wire()->log->message('$eventPage === $getPage ' . ($eventPage === $getPage)); // False
    wire()->log->message('$findPage === $getPage ' . ($findPage === $getPage)); // True

    foreach ($eventPage->parent->children() as $sibling) {
        if ($sibling->id == $eventPage->id) {
            wire()->log->message('$sibling === $eventPage ' . ($sibling === $eventPage)); // False
            wire()->log->message('$sibling === $findPage ' . ($sibling === $findPage)); // True
            wire()->log->message('$sibling === $getPage ' . ($sibling === $getPage)); // True
        }
    }
}

In PW admin, drag/drop the page of id 6778 to change its sort order, and check "messages" logs.
I force getFromCache just to test but it's already default value.

Is it the expected behavior? Why? Am I supposed to modify/save the instance from hook event or from find()? Does it even matter?

[poljpocket]

Hey. Interesting topic.

First, and not to lecture you, Pages::find doesn't have a getFromCache parameter. The find method decides to fetch from cache based on what is queried.

I did a little further digging and code reading. What I can come up with is that the cache gets invalidated every time a save is performed on the page itself or on it's parent. Move operations are handled by the ProcessPageSort module (part of core) and before the sorted hook is called, the functions in this module save the page and after, it's parent. Save operations seem to invalidate the cache from the page down across its children. This makes sense as the database doesn't necessarily hold the same data as the cached versions.

To illustrate this, I have the following hooks with child ID 1017 and parent ID 1015 and performing the same move operation as in your example:

<?php

$wire->addHook(['Pages::saveReady(1017)', 'Pages::save(1017)', 'Pages::sorted(1017)'], function (HookEvent $event) {
    $eventPage = $event->arguments(0);
    $wire = $event->wire;
    $pages = $wire->pages;

    be($event->method . ' - ' . $event->when);

    $findPage = $pages->find(1017)[0];
    $getPage = $pages->get(1017);

    be('$eventPage ' . spl_object_id($eventPage));
    be('$findPage ' . spl_object_id($findPage));
    be('$getPage ' . spl_object_id($getPage));
}, ['before' => true, 'after' => true]);

$wire->addHook(['Pages::save(1015)'], function (HookEvent $event) {
    $wire = $event->wire;
    $pages = $wire->pages;

    be('parent' . ' / ' . $event->method . ' - ' . $event->when);

    $findPage = $pages->find(1017)[0];
    $getPage = $pages->get(1017);

    be('$findPage ' . spl_object_id($findPage));
    be('$getPage ' . spl_object_id($getPage));
}, ['before' => true, 'after' => true]);

This gives me the following event log (the number is the object instance id):

save - before
$eventPage 243
$findPage 243
$getPage 243
saveReady - before
$eventPage 243
$findPage 243
$getPage 243
saveReady - after
$eventPage 243
$findPage 243
$getPage 243
save - after
$eventPage 243
$findPage 187
$getPage 187
parent / save - before
$findPage 187
$getPage 187
parent / save - after
$findPage 196
$getPage 196
sorted - before
$eventPage 243
$findPage 196
$getPage 196
sorted - after
$eventPage 243
$findPage 196
$getPage 196

You can see that after "save - after", the find call loads a new version into cache. The instance IDs change a second time after  "parent / save - after" which must be doing the same. In between these, the IDs stay the same.

This should explain why you do not get your situation on saveReady, but then afterwards in sorted, you do get it since the sort functions save the pages in question before calling the hook.

[da²]

I don't remember exactly why I thought of getFromCache option, probably cause of this in find() documentation:

Quote

loadOptions (array): Optional associative array of options to pass to getById() load options.

and this in getById():

Quote

getFromCache (bool): Allow use of previously cached pages in memory (rather than re-loading it from DB)? (default=true)

But I didn't even specified the key 'loadOptions', and anyway true is default... I had a hard day when I posted this message. ?

About cache cleared after save, in a hook when I save a page (other than the one triggering the hook) I use: 

$page->save(options: ['noHooks' => true, 'uncacheAll' => false]);

It's coming from an advice on this forum, section "Page cache issues...".

So when I saved the last page I suppose cache has not been cleared, but I didn't investigate more since, I just moved this last save() in the function working on instances found via children() (find()) function.

But that may be related with that.

[poljpocket]

The thing is, the ProcessPageSort module is in itself saving the page without any of the options you mentioned. And this is why your problem arises.

But as far as I know, there is nothing wrong with you changing the page in the hook (using the "wrong" page instance) and saving it afterwards in the very same hook. But be careful about this in saveReady or save since this could create an endless loop. Because the page instance gets updated in memory, then saved to the database, your newly created version from find or get has the same data anyway. After the hooks are done, your "old" instance should get discarded. Subsequent finds or gets then will use the cached version.

Also, you always should compare IDs instead of instances of a page, e.g.:

$page1->id === $page2->id;

[da²]

It's getting harder to remember exactly what happened, since code is a bit complex, using multiple hooks, was updated since I posted this topic, and what happens in PW is not totally clear for me.

I just checked something in my new code (fixed one), and find that the changes I do to the page (in sorted hook) are not saved if I don't save it myself, even if sorted is followed by saveReady and so page should be saved. This changes are done on the instance returned by find() (children() run on parent page, exactly).
Maybe I have a cache issue in my code and should investigate more. And maybe I should try to reproduce this in an easy code.

 

7 hours ago, poljpocket said:

you always should compare IDs instead of instances of a page

Sure, I never use instances.

[poljpocket]

I think saving the page again in the sorted hook is no problem and most probably what you must do anyway. What PW does when the user is reordering pages in admin is actually not that complicated:

In module ProcessPageSort the method sortPages gets called whenever an appropriate ajax request is dispatched.

At the very end, both the page and it's parent is saved after the 'sort' property has been updated for the page and subsequent children. This is also why the whole thing shown in the log in one of my last posts happens (line 172+ is shown here).

// trigger hooks
$page->set('sort', $pageSort);
$page->trackChange('sort');
$page->save();
$parent->trackChange('children');
$parent->save();
$this->wire()->pages->sorted($page, false, $changes);

You can see that there will be a saveReady and save hook triggered on the page immediately before the sorted hook is triggered.

[da²]

1 hour ago, poljpocket said:

You can see that there will be a saveReady and save hook triggered on the page immediately before the sorted hook is triggered.

Yes, sorted hook comes after saveReady one, so I have to do a save() myself (or use saveReady and check the sort change myself).

I just reproduced the problem I was talking about in first post:

wire()->addHookAfter('Pages::sorted(6019)', onSorted(...));

function onSorted(HookEvent $event): void
{
    /** @var Page $page */
    $page = $event->arguments(0);

    // This code was in another function used by several hooks.
    // Its job is to update all children in a certain order.
    foreach ($page->parent->children() as $child) {
        if ($child->id == $page->id) {
            $child->title .= "-";
//            $child->save(); // Save new title
        }
    }

    // And this code was in the hook callback.
    $page->save(); // Does not save new title
}

Saving the instance given by hook is not working, I don't know if it's expected, why cache is not giving both times the same instance?

[poljpocket]

As far as I can see and understand in the PW core code, there is no problem with consistency since the $page does contain the most recent version of the data. The issue really is that in the hooks save (after only) and sorted, the cache has been invalidated due to previous database writes, and thus, subsequent queries will generate new instances. This all makes sense. I believe this is to be expected.

In your code, you are updating the title on the "new" instance which is factually another object and then trying to save the "old" page instance. This doesn't work of course. What is wrong with your commented-out line instead of $page->save() outside the loop?

[da²]

OK that's because cache is cleared just before the sorted hook callback, but instance given in callback is the "old" one, yes that makes sense when you look at implementation. ^^

26 minutes ago, poljpocket said:

What is wrong with your commented-out line instead of $page->save() outside the loop?

Nothing is wrong, that's a long story.
Method with children loop is also called by several hooks, including a saveReady, so at first I didn't need to save current page in loop because PW was gonna save it at end.
So when I added sorted hook, I saw that I had to save() the page myself so did it in the hook because other method was not doing it, and that didn't worked.
Finally I refactored this method to take in parameter if it's needed to save current page.

[poljpocket]

Good solution!