Question: Could GridFieldOrderableRows::reorderItems() please use ORM for non-Versioned models?

[chrispenny]

Current implementation

When a model does not implement the Versioned extension, reorderItems() uses SQL queries to update the sort values of models.

When a model does implement the Versioned extension, reorderItems() sets the sort value on the record and performs a write() through the ORM.

Problem statement

When developers implement onBeforeWrite() and onAfterWrite() methods, there is some confusion on whether or not these methods will be called when the records are reordered. If they are Versioned, then these methods will be triggered; if they are not Versioned, then they will not.

Question

Could we please standarise the reorderItems() method to always use the ORM to save records?

Considerations

Would this sort of change need to be a new major version, or would it be considered a "bugfix"?

[emteknetnz]

Code being referred to is here

Unversioned SQL

silverstripe-gridfieldextensions/src/GridFieldOrderableRows.php

Line 632 in 6d6cf4a

'UPDATE "%s" SET "%s" = %d%s WHERE %s',

Versioned ORM sort

silverstripe-gridfieldextensions/src/GridFieldOrderableRows.php

Line 660 in 6d6cf4a

$record->$sortField = $newSortValue;

[emteknetnz]

I'd call it an enhancement (ENH prefix).

I don't see why this would need a new major. New minor seems fine since there would be a behavioral change as $record->write() would now be getting called, instead of the current behavior on not called with the stealth update via raw SQL.

Happy to peer review if you can provide a PR. There's some existing tests in GridFieldOrderableRowsTest.php, though you may need to add to these depending on existing coverage. Looks like there a few use cases listed within inline comments in reorderItems() that will need to be considered

[chrispenny]

Perfect! Thanks, @emteknetnz. I will work on that PR soon 😄