Follow up for #1498674: Refactor node properties to multilingual

Problem/Motivation

Simplify code

In 1498674 was the @todo

-      // @todo: Make the {node_revision}.log column nullable so that we can

On the line in the patch in comment #303:
503

The line of the final patch might change but that info might help to find them.

For example, 

+      // must be set because {node_field_revision}.log is a text column and
+      // therefore cannot have a default value. However, it might not be set at
+      // this point (for example, if the user submitting a node form does not
+      // have permission to create revisions), so we ensure that it is at least
+      // an empty string in that case.
+      // @todo Make the {node_field_revision}.log column nullable so that we
+      //   can remove this check.
       if (!isset($record->log)) {
         $record->log = '';
       }
     }

Proposed resolution

Make the {node_field_revision}.log column nullable so that we can remove this check.

User interface changes

No.

API changes

No.

CommentFileSizeAuthor
#13 make-column-nullable-2004366-13.patch1.28 KBInternetDevels
#11 2004366-make_the_node_field_revision_log_nullable-11.patch1.84 KBAlbert Volkman
#5 2004366-make_the_node_field_revision_log_nullable-4.patch1.95 KBkalabro
#2 2004366-make_the_node_field_revision_log_nullable-2.patch2.03 KBkalabro
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kalabro’s picture

kalabro
she/her
CreditAttribution: kalabro commented
Assigned: Unassigned » kalabro

#CodeSprintUA 2013 Online team will work on it today.

kalabro’s picture

kalabro
she/her
CreditAttribution: kalabro commented
Assigned: kalabro » Unassigned
Status: Active » Needs review
Issue tags: +CodeSprintUA
FileSize
2004366-make_the_node_field_revision_log_nullable-2.patch2.03 KB

1. Db column {node_field_revision}.log already nullable ('not null' => FALSE) via #1498674: Refactor node properties to multilingual
2. $record->log is always set, because we call mapToRevisionStorageRecord() before preSaveRevision().

Just removed conditional statement.

podarok’s picture

podarok
🇺🇦 he/him/his
Primary language Ukrainian
Location Ukraine
CreditAttribution: podarok commented
Status: Needs review » Reviewed & tested by the community

#2 looks good
RTBC

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/node/lib/Drupal/node/NodeStorageController.phpundefined
@@ -135,26 +135,15 @@ protected function preSave(EntityInterface $node) {
+    if (!$entity->isNewRevision()) {
+      if (isset($entity->original) && (!isset($record->log) || $record->log === '')) {
+        // If we are updating an existing node without adding a new revision, we
+        // need to make sure $entity->log is reset whenever it is empty.
+        // Therefore, this code allows us to avoid clobbering an existing log
+        // entry with an empty one.
+        $record->log = $entity->original->log;
       }
     }

The two ifs can be collasped to one...

kalabro’s picture

kalabro
she/her
CreditAttribution: kalabro commented
Status: Needs work » Needs review
FileSize
2004366-make_the_node_field_revision_log_nullable-4.patch1.95 KB

The two ifs can be collasped to one...

Done!

Status: Needs review » Needs work

The last submitted patch, 2004366-make_the_node_field_revision_log_nullable-4.patch, failed testing.

kalabro’s picture

kalabro
she/her
CreditAttribution: kalabro commented

Hmm.. Only config storage exceptions.

exception 'Drupal\Core\Config\StorageException' with message 'Failed to write configuration file ...

kalabro’s picture

kalabro
she/her
CreditAttribution: kalabro commented

Hmm.. Only config storage exceptions.

exception 'Drupal\Core\Config\StorageException' with message 'Failed to write configuration file ...

kalabro’s picture

kalabro
she/her
CreditAttribution: kalabro commented
Status: Needs work » Needs review

#5: 2004366-make_the_node_field_revision_log_nullable-4.patch queued for re-testing.

jair’s picture

jair CreditAttribution: jair commented
Issue tags: +Needs reroll
Albert Volkman’s picture

Albert Volkman CreditAttribution: Albert Volkman commented
FileSize
2004366-make_the_node_field_revision_log_nullable-11.patch1.84 KB

Re-roll.

The last submitted patch, 2004366-make_the_node_field_revision_log_nullable-11.patch, failed testing.

InternetDevels’s picture

InternetDevels CreditAttribution: InternetDevels commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
make-column-nullable-2004366-13.patch1.28 KB

The file was changed and we can not apply old patch, so we did not create interdiff

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue tags: -Needs reroll

Removing reroll tag. Thanks!

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs a reroll.

Albert Volkman’s picture

Albert Volkman CreditAttribution: Albert Volkman commented

Is this still relevant?

http://cgit.drupalcode.org/drupal/tree/core/modules/node/src/Entity/Node...

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
Status: Needs work » Closed (fixed)

Looks like that code has been removed already per #16.