NEW Auto-publish objects on sort #292
Conversation
There was a problem hiding this comment.
A few comments. Also this is essentially a backport of a change in the SS4 branch of this module - the difference is that it is opt-in configuration. I think that you should do the same - we don't want to immediately change the functionality here because you're not actually fixing a bug.
| @@ -463,6 +463,9 @@ protected function reorderItems($list, array $values, array $sortedIDs) { | |||
| if ($class == $this->getSortTable($list)) { | |||
| $isVersioned = $class::has_extension('Versioned'); | |||
| } | |||
| if ($class::has_extension('Versioned')) { | |||
| $isVersioned = true; | |||
There was a problem hiding this comment.
I assume you added this to use it later but it's unused?
| @@ -505,11 +508,15 @@ protected function reorderItems($list, array $values, array $sortedIDs) { | |||
| // similar to the SiteTree where you change the position, and then | |||
| // you go into the record and publish it. | |||
| foreach($sortedIDs as $sortValue => $id) { | |||
| if($map[$id] != $sortValue) { | |||
| $record = $class::get()->byID($id); | |||
| $record = $class::get()->byID($id); | |||
There was a problem hiding this comment.
It seems like this could be left inside the condition below? It's a sliiiight performance impact moving it out of the condition because you're loading a DataObject for every item regardless of the following condition...
| $record->$sortField = $sortValue; | ||
| $record->write(); | ||
| } | ||
| // Update stage to live only for published records | ||
| if ($record->isPublished()) { |
There was a problem hiding this comment.
This is only relevant if there's a write, so you can move it into the condition. Also this is probably where you want to use $isVersioned you prepared earlier.
ScopeyNZ
changed the title
|
I agree, I will change my code. |
|
Closing this pr, as this issue does not exists in fresh SS3 installation. |
Related SS4 PR: #289