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

fix: getPage returning the wrong page #1082

Merged
merged 1 commit into from
Sep 21, 2023
Merged

fix: getPage returning the wrong page #1082

merged 1 commit into from
Sep 21, 2023

Conversation

wilr
Copy link
Member

@wilr wilr commented Aug 17, 2023

If you programaticaly update the ParentID of an element, getPage() still returns the outdated page.

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the key used in cacheData could break customisations in subclasses which fetch or set data into cacheData, since it's not a private property.

Is there a sensible way to bust the cache instead? e.g. in onBeforeWrite(), or - if you want to call getPage() before writing for some reason, check if $this->isChanged('PageID') and don't use the cached data if it has been changed?

This also seems like a good candidate for a unit test to avoid regressions.

@wilr
Copy link
Member Author

wilr commented Aug 17, 2023

@GuySartorelli Updated to use isChanged() now and added a unit test.

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally the unit test would also test the caching functionality - but given that functionality was already there it wouldn't be reasonable to ask for this PR to introduce that test coverage. I'm okay with this as is.

Looks good, will merge when CI goes green. Thanks for submitting this.

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is unhappy - please fix the test.

tests/BaseElementTest.php Outdated Show resolved Hide resolved
@GuySartorelli
Copy link
Member

CI is still unhappy here. Can you please resolve that?

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are, again, a lot of unrelated changes in this PR related to formatting.
I am going to ask you, again, that in future pull requests you either don't commit those changes, or you include them in a separate commit. I'm going to start asking you to make those changes before merging PRs if you continue to ignore that request. You can leave it as is for this one though.

Updated to use isChanged() now and added a unit test.

This doesn't seem to be the case... did you change it again since then? If so, can you please explain why? Using isChanged() seems like the perfect solution here instead of adding more info into cacheData.

@wilr
Copy link
Member Author

wilr commented Sep 20, 2023

Hi @GuySartorelli isChanged() doesn't work as it was cleared when calling onBeforeWrite() so I reverted back (but to a new value to keep your original comment in mind). What we were seeing

echo $element->getPage()->Title // 'Contact us';
$element->ParentID = 1;
$element->write();
echo $element->getPage()->Title // 'Contact us';

As $element->isChanged('ParentID') == false. We could clear the cache data onBeforeWrite but that seemed like leaking things outside of that method.

Sorry! Editor being all auto-magic fix. I'll turn it off

@GuySartorelli
Copy link
Member

GuySartorelli commented Sep 20, 2023

Thanks for that context. In that case, I think this is good as is. Once CI has gone green on this latest push I'll approve and merge.

Sorry! Editor being all auto-magic fix. I'll turn it off

You can keep it on if you like, so long as any changes it makes are in a separate commit (e.g. save the page without making your changes, and if there's a change caused by your IDE, commit that result first and then make your PR changes) - though I could see that workflow being a pain to remember to do, especially if it differs from your regular workflow.

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

@GuySartorelli GuySartorelli merged commit 4506c15 into silverstripe:5 Sep 21, 2023
12 checks passed
@rasstislav
Copy link

Hi @GuySartorelli. Hi @wilr.

This issue also exists v4. Is it possible to backport this patch commit to this version as well?

Thanks

@GuySartorelli
Copy link
Member

@rasstislav Absolutely.
The easiest way to do that would be to just open a new PR introducing this change to the 4.11 branch.

@rasstislav
Copy link

@GuySartorelli ready, thanks :)

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

Successfully merging this pull request may close these issues.

3 participants