Sorting blocks in draft mode can cause changes on the frontend as it updates the Element_Live table sort value

[Jianbinzhu]

Module version(s) affected

4.11.0

Description

When re-order the blocks without publishing the page, I would expect no changes to the frontend. So I looked into the sort values in the database and finds out that sorting blocks in draft mode also updates the Element_Live table.

How to reproduce

I have created some dummie data to re-create the bug, before re-ordering

Both Element & Element_Live table look like this

the following data is after moving content block 8,9,10 up below content block 1.

The Element table

Select `ID`, `Title`, `Sort` FROM `Element` ORDER BY `Sort` ASC;

The Element_Live table

Select `ID`, `Title`, `Sort` FROM `Element_Live` ORDER BY `Sort` ASC;

On the front-end, the content block 7 is under content block 10, because they both have the same sort value on the Element_Live table, the next thing it sorts by if they both have the same sort value is the ID, in this case content block 7 has a higher ID than content block 10.

Possible Solution

No response

Additional Context

No response

Validations

• Check that there isn't already an issue that reports the same bug
• Double check that your reproduction steps work in a fresh installation of silverstripe/installer (with any code examples you've provided)

Acceptance criteria

• First validate if the sort order goes straight to live in symbiote gridfieldextensions
• (assuming it doesn't) remove the straight to live logic in CMS 5.3

PRs

• FIX Don't update sort order in live until a block has been published #1234
• DOC Document change to sorting elemental blocks developer-docs#554

[emteknetnz]

The reported version is for CMS 4 which is currently only receiving security patches for high and critical vulnerabilities as per our support timeline so we won't look to fix this unless it's still an issue for CMS 5.2 - do you know if it's still an issue in CMS 5.2?

[Jianbinzhu]

The reported version is for CMS 4 which is currently only receiving security patches for high and critical vulnerabilities as per our support timeline so we won't look to fix this unless it's still an issue for CMS 5.2 - do you know if it's still an issue in CMS 5.2?

I have just replicate this issue on silverstripe-framework: 5.2.8 and silverstripe-element: 5.2.3

[emteknetnz]

Have replicated

It's an issue with the graphql SortBlockMutation, which I think is auto-scaffolded.

We're currently in the process of removing graphql from the CMS for CMS 6.

We shouldn't be spending any time fixing graphql endpoints in the cms at this point, so the fix here would to copy in the ElementalAreaController::apiSort() method from the relevant CMS 6 PR into CMS 5 and use lib/Backend to fetch from it

The logic for this is in DNADesign\Elemental\Services\ReorderElements which is unchanged for CMS 6, so fix it there

[mfendeksilverstripe]

@Jianbinzhu

Managed to reproduce this issue as well. Here are the details I've found (tested on 4.13.44 of the CMS):

It looks like the reorder auto-publishes the changes on alll other blocks that were impacted by the block reorder except the one block that was moved around.

This is how it looks like after reorder:

Note the red circle on the Block 2 which was dragged down. It's in modified on draft stage while the other block reports published.

Draft table data looks ok.

Live table data is incorrect as we have a mix of data present (one block was published and the other was not).

The data gets corrected after publishing the page though.

[mfendeksilverstripe]

@Jianbinzhu The issue is located in ReorderElements:

// Update both the draft and live versions of the records
$tableNames = [$baseTableName];
if (BaseElement::has_extension(Versioned::class)) {
    /** @var BaseElement&Versioned $element */
    $tableNames[] = $element->stageTable($baseTableName, Versioned::LIVE);
}

Looks like this behaviour is intentional although I can't really understand why. Either way you should be able to use Injector to override this class and disable this behaviour so you can at least fix this on your project.

[jakxnz]

Would someone be willing to raise an RFC on what the future behaviour for this logic should be to better match how users prefer/need the module to behave out-of-the-box?

[emteknetnz]

I'm not sure that an RFC is really something needed here. Draft changes are going straight to live on a versioned dataobject without the user first clicking a publish button.

I think it's fair to assume that content editors would find this to be an unexpected behaviour

I think should just be treated as a regular bug.

[emteknetnz]

I've had a look around at the history of "why" things are like this - the issue is here (as a comment on a related though slightly different original issue) - the code went in this pr and a link was made on that PR to a long research/discussion piece No clear way to handle sorting with versioned object

At the end of that long piece there a Ingo commented that it was "... a long standing issue, which can be remedied from an author perspective by simply publishing everything ..."

After all of that I'm still not quite sure why in elemental it's is directly updating the _Live table though. I don't think that should be happening.

There was a comment on the PR "Note this functionality is similar to SiteTree in CMS but is still inherently flawed" - that's now false, I did a quick test reordering the main tree in the CMS that list pages and the _Live table is not updated, on draft. So we should update elemental to match that behaviour.

[Jianbinzhu]

@Jianbinzhu The issue is located in ReorderElements:

// Update both the draft and live versions of the records
$tableNames = [$baseTableName];
if (BaseElement::has_extension(Versioned::class)) {
    /** @var BaseElement&Versioned $element */
    $tableNames[] = $element->stageTable($baseTableName, Versioned::LIVE);
}

Looks like this behaviour is intentional although I can't really understand why. Either way you should be able to use Injector to override this class and disable this behaviour so you can at least fix this on your project.

@mfendeksilverstripe Looks like disabling this behaviour causes the sort values of the blocks between the Element table and the Element_Live table to be out of sync when the block/page is published. I think we need to re-sync the sort values upon publishing the block/page.

@emteknetnz do you think this is the right approach to this issue? ☝️

[emteknetnz]

Yeah those are the lines writing to the live table on a draft save.

I'd expect that you'd simply reduce it to $tableNames = [$baseTableName];.

causes the sort values of the blocks between the Element table and the Element_Live table to be out of sync when the block/page is published

Yeah, that'll happen because only blocks that have modified changes will be published. I think the ReorderElements doesn't actually write new versions, instead it just updates the Sort column directly. I think you'd need to make other changes to the elements so that they get published on page save so that the new Sort order goes live

You could probably make a case that all the blocks that get their Sort order updated by ReorderElements should just get the regular ORM $block->write() treatment on sort so they all get new versions. That should ensure that page publishing works as expected.

[GuySartorelli]

I'm guessing that the low-level SQL UPDATE was used to avoid calling write() primarily for performance reasons, but there's no code comments indicating the reasoning.

It's worth noting that GridFieldSortableRows and GridFieldOrderableRows both just loop through all the records and write them, so that versions are handled by the ORM.

SiteTree is a little weird and is probably what motivated the way this is implemented for elemental.
CMSMain only calls write() on the specific record you're dragging, and uses low-level SQL UPDATE for everything else: https://github.com/silverstripe/silverstripe-cms/blob/81bfa84f0a43048668df67afa2018f162cc9115f/code/Controllers/CMSMain.php#L825-L845
This is the same thing elemental does.
The difference is in SiteTree::onAfterPublish() where another low-level SQL UPDATE is run to update all sibling published pages sort values to match their draft values. So publishing one page publishes the sort order for all sibling pages.

Note that there are advantages and disadvantages to both approaches.

If you call write() on all records, you create new version history for all records. This is a little slower but is more correct.
On the other hand, this means you can publish a few records, and revert changes from a few other records, which could result in duplicate sort values across your records.
The way that SiteTree handles this means the version history is slightly incorrect (which is true for elemental right now anyway), but doesn't open itself to the same inconsistencies.

[GuySartorelli]

@Jianbinzhu Please test #1234 and see if it fits your needs

[Jianbinzhu]

@Jianbinzhu Please test #1234 and see if it fits your needs

@GuySartorelli this is working like a gem 😁

[emteknetnz]

Linked PRs have been merged, they will be released in CMS 5.3

[michalkleiner]

@GuySartorelli in general, do we need to consider there might be potentially other changes to the sibling records that we don't want written/published and where a low-level SQL update would be better? That could be the reason why SiteTree does it that way and would be probably also what I would prefer, unless we had a mechanism where we could tell write or publish to only look at certain fields and leave others untouched.

EDIT:
Never mind, checked the PR and it's done via the SQL update, so that's all good.