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: Remove legacy fields which prevent page publish (fixes #2455) #3002

Merged

Conversation

kinglozzer
Copy link
Member

@kinglozzer kinglozzer commented Sep 11, 2024

Description

This updates the list of fields which are ignored when deciding whether a new version (and therefore a new live version) should be created. Also made it configurable while I was there.

Manual testing steps

Create a page type with a field Status, save and publish. Then change only the status field and publish again: a new version won’t be written, and the live site won’t update. If you change status and something else, it will.

Issues

Pull request checklist

  • The target branch is correct
  • All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
    • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
  • The commit messages follow our commit message guidelines
  • The PR follows our contribution guidelines
  • Code changes follow our coding conventions
  • This change is covered with tests (or tests aren't necessary for this change)
  • Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
  • CI is green

@@ -1693,7 +1703,7 @@ protected function onBeforeWrite()
}

// Check to see if we've only altered fields that shouldn't affect versioning
$fieldsIgnoredByVersioning = ['HasBrokenLink', 'Status', 'HasBrokenFile', 'ToDo', 'VersionID', 'SaveCount'];
Copy link
Member Author

Choose a reason for hiding this comment

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

HasBrokenFile and HasBrokenLink are the only two fields that actually exist anymore.

I thought about changing VersionID to Version (which is what the field is now named), but that would be a behavioural change. It would also be a bit weird: changing the version number in the SiteTree table without writing a version would mean that SiteTree and SiteTree_Versions could end up out of sync.

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.

Overall looks good, thanks!

Can you please retarget this to 5.2? That branch is still supported for bug fixes so it'd be good to get the fix in there instead of having to wait for 5.3.0 stable to release.

code/Model/SiteTree.php Outdated Show resolved Hide resolved
@kinglozzer kinglozzer changed the base branch from 5.3 to 5.2 September 12, 2024 08:15
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.

LGTM, works well locally.

@GuySartorelli GuySartorelli merged commit c77a4c9 into silverstripe:5.2 Sep 12, 2024
15 checks passed
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.

2 participants