Contrib.social

RevisionableInterface::preSaveRevision() should move to the storage layer

Open on Drupal.org →
Created on 3 July 2015, over 9 years ago
Updated 21 February 2024, 9 months ago

Problem/Motivation

The signature of RevisionableInterface::preSaveRevision() is SQL-specific as it has a $record parameter, that wouldn't make sense for a MongoDB implementation.
That was introduced only to address an obscure issue with the revision_log field:

    if (!$this->isNewRevision() && isset($this->original) && (!isset($record->revision_log) || $record->revision_log === '')) {
      // If we are updating an existing node without adding a new revision, we
      // need to make sure $entity->revision_log is reset whenever it is empty.
      // Therefore, this code allows us to avoid clobbering an existing log
      // entry with an empty one.
      $record->revision_log = $this->original->revision_log->value;
    }

Proposed resolution

  • Move this logic to the storage layer and address it "generically" for any entity type.
  • Remove RevisionableInterface::preSaveRevision() or at least make it not SQL-specific.

Remaining tasks

  • Validate the proposed solution
  • Write a patch
  • Review it

User interface changes

None

API changes

Sort of BC API change: the logic will be moved where it can always be executed for any entity type. Then it should be safe to remove the method, since there are probably not many use cases for it left. We could also leave an empty implementation in place to be strictly BC.

Data model changes

None

📌 Task
Status

Active

Version

11.0 🔥

Component
Entity  →

Last updated about 5 hours ago

Created by

🇮🇹Italy plach Venezia

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment 9 months ago →
    🇬🇧United Kingdom longwave UK

    Just run into this in a roundabout way. I have a site which has altered the revision_log field and $record->revision_log is not a valid column in the table any more. Agree that this is making assumptions about the data structure that is in use here, and it should be done in a more generic way instead.

  • Comment 9 months ago →
    🇬🇧United Kingdom longwave UK
    if (!$this->isNewRevision() && isset($this->original) && (!isset($record->revision_log) || $record->revision_log === '')) {

    I also think this logic could be simplified:

    if (!$this->isNewRevision() && isset($this->original) && isset($record->revision_log) && $record->revision_log === '') {

    If $record->revision_log is not set then we don't need to set it at all for the purposes of the SQL statement.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024