Rename the 'log' field to 'revision_log' in Node and CustomBlock

Let's see what breaks.

@marco: Are you still working on this?

Wow, @marco. Awesome work! I would have never gotten this patch green in only 2 tries. :-) Respect. And thanks!

I have a few minor remarks, but this is really close, I think. It would be awesome if you could finish this off?! :-)

1. +++ b/core/modules/block/custom_block/custom_block.install
   @@ -99,10 +99,10 @@ function custom_block_schema() {
   -        'not null' => TRUE,
   +        'not null' => FALSE,
   
   +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Entity/CustomBlock.php
   @@ -114,23 +114,11 @@ public function getInstances() {
   -      // When inserting either a new custom block or a new custom_block
   -      // revision, $entity->log must be set because {block_custom_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
   -      // the 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 {block_custom_revision}.log column nullable so that we
   -      // can remove this check.
   -      if (!isset($record->log)) {
   -        $record->log = '';
   -      }
   

   Sure, let's do it! Nice find.

2. +++ b/core/modules/block/custom_block/js/custom_block.js
   @@ -18,7 +18,7 @@
   -        if (revisionCheckbox.is(':checked') || (!revisionCheckbox.length && $context.find('.form-item-log textarea').length)) {
   +        if (revisionCheckbox.is(':checked') || (!revisionCheckbox.length && $context.find('.form-item-revision-log textarea').length)) {
   

   Wow, good catch!!! This means this needs manual testing, however. Tagging accordingly.

3. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Entity/CustomBlock.php
   @@ -186,9 +174,9 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   -      ->setDescription(t('The revision log message.'))
   +      ->setDescription(t('The log entry explaining the changes in this revision.'))
   

   Thanks for making the description consistent with Node. Yay!

4. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockRevisionsTest.php
   @@ -45,24 +45,24 @@ protected function setUp() {
   -    $logs = array();
   +    $revisionLogs = array();
   ...
   -    $logs[] = '';
   +    $revisionLogs[] = '';
   ...
   -      $logs[] = $block->getRevisionLog();
   +      $revisionLogs[] = $block->getRevisionLog();
   ...
   -    $this->logs = $logs;
   +    $this->revisionLogs = $revisionLogs;
   
   @@ -70,13 +70,13 @@ protected function setUp() {
   -    $logs = $this->logs;
   +    $revisionLogs = $this->revisionLogs;
   ...
   +      // Verify revision log is the same.
   +      $this->assertEqual($loaded->getRevisionLog(), $revisionLogs[$delta], format_string('Correct log message found for revision !revision', array(
   

   Same as above. Either leave the local variable $logs as is, or rename it to $revision_logs. (Again $this->revisionLogs should not be $this->revision_logs.)

5. +++ b/core/modules/node/lib/Drupal/node/Entity/Node.php
   @@ -68,25 +68,12 @@ class Node extends ContentEntityBase implements NodeInterface {
   -      // When inserting either a new node or a new node revision, $node->log
   -      // 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 = '';
   -      }
   

   Checked that the log field on {node_revision} (not {node_field_revision} as the comment says is already 'not null' => FALSE.

6. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeRevisionsAllTestCase.php
   @@ -47,7 +47,7 @@ function setUp() {
   -    $logs = array();
   +    $revisionLogs = array();
   
   @@ -55,7 +55,7 @@ function setUp() {
   -      $logs[] = $node->log = $this->randomName(32);
   +      $revisionLogs[] = $node->revision_log = $this->randomName(32);
   
   @@ -71,7 +71,7 @@ function setUp() {
   -    $this->logs = $logs;
   +    $this->revisionLogs = $revisionLogs;
   
   @@ -79,7 +79,7 @@ function setUp() {
   -    $logs = $this->logs;
   +    $revisionLogs = $this->revisionLogs;
   
   @@ -100,10 +100,11 @@ function testRevisions() {
   -    foreach ($logs as $log) {
   ...
   +    foreach ($revisionLogs as $revision_log) {
   

   If you want to rename the local variables as well (keeping them would have been fine as well) the local variable should be named $revision_logs. Note that the member variable is named correctly. To be clear: The third hunk of the above excerpt should then read:
   $this->revisionLogs = $revision_logs;

7. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeRevisionsAllTestCase.php
   @@ -100,10 +100,11 @@ function testRevisions() {
   +    // Confirm the correct revision log message appears on "revisions overview"
   +    // page.
   

   Since we're changing this already can you add a the between on and "revisions overview"? That might mean you would have to re-adjust the wrapping, though.

8. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeRevisionsTest.php
   @@ -47,7 +47,7 @@ function setUp() {
   -    $logs = array();
   +    $revisionLogs = array();
   

   Again...

9. +++ b/core/modules/node/node.js
   @@ -18,7 +18,7 @@
   -        if (revisionCheckbox.is(':checked') || (!revisionCheckbox.length && $context.find('.form-item-log textarea').length)) {
   +        if (revisionCheckbox.is(':checked') || (!revisionCheckbox.length && $context.find('.form-item-revision-log textarea').length)) {
   

   Again, this needs manual testing. Great job for finding this, though!

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Entity/CustomBlock.php
@@ -114,23 +114,11 @@ public function getInstances() {
-    if ($this->isNewRevision()) {
...
-    elseif (isset($this->original) && (!isset($record->log) || $record->log === '')) {
+    if (!$this->isNewRevision() && isset($this->original) && (!isset($record->revision_log) || $record->revision_log === '')) {

+++ b/core/modules/node/lib/Drupal/node/Entity/Node.php
@@ -68,25 +68,12 @@ class Node extends ContentEntityBase implements NodeInterface {
-    if ($this->newRevision) {
...
-    elseif (isset($this->original) && (!isset($record->log) || $record->log === '')) {
+    if (!$this->newRevision && isset($this->original) && (!isset($record->revision_log) || $record->revision_log === '')) {

Well the condition is just moved from the if() condition. So I disagree on the need for additional test coverage here. The logic remains the same.

I agree that we should be using isNewRevision() (i.e. the method call) in both places for consistency.

Reroll after #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities.

Which reminds me that we should also now resolve the @todo there, that spawned this issue :-)

I consider that interdiff to be part of the merge, so straight back to RTBC :-)

Wow, ContentEntityDatabaseStorage has quite some thorough test coverage these days... :-P