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

BUG: Page parentage update fix. #136

Merged
merged 2 commits into from
May 10, 2022
Merged

BUG: Page parentage update fix. #136

merged 2 commits into from
May 10, 2022

Conversation

mfendeksilverstripe
Copy link
Collaborator

@mfendeksilverstripe mfendeksilverstripe commented May 10, 2022

SiteTree re-organization fix

The condition did not properly match sometimes due to the type comparison which ended up causing unwanted purge static cache jobs.

Workaround for silverstripe/silverstripe-versioned#360

Split off from #132

@dhensby
Copy link
Contributor

dhensby commented May 10, 2022

@mfendeksilverstripe can you make sure you open PRs from forks in the future, please.

As for this change, we might as well use loose compactors rather than explicit casting in this case? Or is there some benefit to explicitly casting as ints? As a personal peev, I dislike the assumption that DB IDs should be ints because it really ties our hands in the future

@michalkleiner
Copy link
Contributor

I'd second Dan's opinion here, allowing string IDs in future to support e.g. UUIDs instead of sequential numeric IDs will need to be a thing, so using loose comparison here would be OK from my perspective, or, if we have to have a strict comparison, going the other way and comparing it as strings.

@mfendeksilverstripe
Copy link
Collaborator Author

Switched to loose comparison as per your recommendation @michalkleiner .

cc @dhensby

@dhensby dhensby merged commit 9da96cc into master May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants