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

Circular reference mitigation #258

Closed
wants to merge 1 commit into from
Closed

Circular reference mitigation #258

wants to merge 1 commit into from

Conversation

KeeneState
Copy link

Hi Ryan, we ran into a series of out of memory errors during page saves after a page was mistakenly self-referenced, e.g. $page->FieldtypePage[Select] = $page. This took care of it, though I'm not certain it's the best place to prevent it.


Adding self check to prevent circular reference on page save leading to OOM. This will occur in cases where a Page contains a Page select input, with $this selected. While this would likely be due to a administration mistake or the result of poor template design, it will white-screen any page save containing the field as it attempts to uncache.

Adding self check to prevent circular reference on page save leading to OOM. This will occur in cases where a Page contains a Page select input, with $this selected. While likely a mistake or the result of poor template design, it will white-screen any page save containing the field as it attempts to uncache. If such a reference should instead be illegal, throwing an error at this point could be the way to go?
@apeisa
Copy link
Contributor

apeisa commented Nov 29, 2013

I did hit this same problem with my https://github.com/apeisa/UserGroups module, where self-reference is required and intended.

@KeeneState
Copy link
Author

Antti, I deleted my fork and Ryan's response to this pull request was removed along with it - still getting used to some of the eccentricities of github. Ryan actually had an improved version of the patch he was going to apply to dev.

@KeeneState KeeneState closed this Dec 2, 2013
@apeisa
Copy link
Contributor

apeisa commented Dec 6, 2013

Thanks for letting me know @KeeneState

@ryancramerdesign is this fix already in place in dev?

@ryancramerdesign
Copy link
Owner

A fix was put on dev, but I'm not positive I committed it in full–since
working with your module, I've backtracked and have been trying to get it
working in full with circular references, rather than prevent them. I'm not
there yet, but do think we'll get there.

On Fri, Dec 6, 2013 at 4:04 PM, Antti Peisa notifications@github.comwrote:

Thanks for letting me know @KeeneState https://github.com/KeeneState

@ryancramerdesign https://github.com/ryancramerdesign is this fix
already in place in dev?


Reply to this email directly or view it on GitHubhttps://github.com//pull/258#issuecomment-30029298
.

@apeisa
Copy link
Contributor

apeisa commented Dec 8, 2013

Thanks for the information ryan. Hopefully this get's fixed soon. I got project in hand where page based permission would be a lifesaver.

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.

None yet

3 participants