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

Remove use of ChangeSet from RecursivePublishable #472

Open
3 tasks
emteknetnz opened this issue Sep 5, 2024 · 5 comments
Open
3 tasks

Remove use of ChangeSet from RecursivePublishable #472

emteknetnz opened this issue Sep 5, 2024 · 5 comments

Comments

@emteknetnz
Copy link
Member

emteknetnz commented Sep 5, 2024

RecursivePublishable::publishRecursive() will create a ChangeSet + (child ChangeSetItem indirectly) whenever any object is published. This is at least partially responsible for the large ChangeSet/ChangeSetItem database tables that some websites have.

The ChangeSet functionality is used primarily for silverstripe/campaign-admin. It's believed that the ChangeSet were originally intended to be a more core part of Versioning and facilitate rollbacks, though that's not how rollbacks are implemented right now and it seems very unlikely we'd switch to using ChangeSets

It seems like we should simply remove the creation of a ChangeSet/ChangeSetItem in CMS 6 since the database records created are pretty much pointless

Acceptance criteria

  • Stop creating ChangeSet/ChangeSetItem records in RecursivePublishable::publishRecursive()
  • ChangeSet_* tables are no longer created by default i.e. delete the ChangeSet_* DataObjects
  • Changelog calls out that existing tables will no longer be updated in CMS 6

Kitchen sink CI

PRs

@mfendeksilverstripe
Copy link
Contributor

Fixes #450

@emteknetnz
Copy link
Member Author

emteknetnz commented Feb 13, 2025

I've had a go at this by essentially just getting the recursively owns objects and calling publishSingle() on them, and then "manually" removing any objects that were in a live only state before calling publishRecursive().

However a handful of unit tests in kitchen sink broke, it seems like there's some difference in how ->Version is "calculated" and thus there's actually big scope to really mess up data integrity here, so I think this approach may be just too risky.

The existing implementation does require the ChangeSet + ChangeSetItem DataObjects to be written to the database. Given this there's no scope to attempt to increase publishing performance by not writing these records, at least not short-term

I might make another PR where we just delete the ChangeSet + ChangeSetItem DataObjects after using them for a recurisive publish, to simply prevent database bloat

Update: OK that doesn't work either VersionedOwnershipTest::testUnversionedPublish() fails. Oh well.

@emteknetnz emteknetnz closed this as not planned Won't fix, can't repro, duplicate, stale Feb 13, 2025
@mfendeksilverstripe
Copy link
Contributor

@emteknetnz Your solution differs from what's I've proposed in #450 . I'll try to put up a PR and see what difference does it make. They key bit here is DBDatetime::withFixedNow which ensures that the versioned records have correct timestamps.

I'll have a another look at this one later.

@GuySartorelli
Copy link
Member

Reopening because even if this doesn't get done in time for CMS 6, it's definitely worth looking into for a future major release.

@mfendeksilverstripe
Copy link
Contributor

Encountered the same issue on my test PR: #493

Not sure what the issue is but I'm guessing it may be some undocumented behaviour.I suggest to do a data compare in terms of version records and see what's the difference in terms of raw data. That might help to locate the root cause.

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

No branches or pull requests

3 participants