[6.x] Fix entries accepting a cyclic origin - #15288
Open
lwekuiper wants to merge 2 commits into
Open
Conversation
Contributor
|
Overlaps with some fixes in this PR #15253 |
Contributor
Author
|
Thanks, hadn't seen that one. I ran the #15253 branch against a self-origin and against a cycle that's already in the Stache, and both still overflow the stack there: the store calls |
Contributor
|
No worries, thanks for the PR! I'll let Jason decide how it should be handled – just wanted to flag it :) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An entry's origin can be pointed at an entry that already originates from it. Nothing rejects this, the cycle is written to disk, and from then on loading either entry, or saving anything else in that collection, recurses until PHP dies.
stache:cleardoesn't recover from it either: the markdown needs hand editing plus removing the cached Stache files.Reproduced on 6.29.0, file driver, multi-site, PHP 8.4:
The control panel can't produce this, but the API accepts it, so imports and content sync scripts can.
Cause
Resolving an origin runs a query, and the Stache calls
syncOriginal()on every item it hands out, which resolved the origin again throughgetCurrentDirtyStateAttributes(). For a cyclic pair that never bottoms out:root()andancestors()also loop forever on a cycle.Fix
Entry::save()throwsEntryOriginRecursionExceptionwhen the origin would close a loop: directly, through another entry, or on itself. An origin pointing at an entry that no longer exists still saves, as before.getCurrentDirtyStateAttributes()uses the origin id it already holds instead of resolving it, so dirty state tracking no longer queries from inside the store.root()andancestors()walk the chain with a visited set instead of followingorigin()blindly.descendants()already does this for the other direction.The check lives in
save()rather than inorigin()on purpose. The Stache calls the setter when hydrating from disk, so rejecting there would make an already broken site throw on load instead of letting you repair it.Not in this PR
This keeps new cycles from being written. It doesn't make a site that already has one on disk usable again:
value(),blueprint(),template(),layout()anddate()each followorigin()recursively, andblueprint()is called for every entry whenever an index is rebuilt. Happy to do that in a follow-up, but I'd like to hear first whether you'd rather guard each of those reads or normalise bad origins when the Stache loads them.Tests cover the three rejection cases, the dangling origin case, and
root()/ancestors()on a cycle.