Add changed time tracking to custom blocks

Ah, sorry, should have thought of this before. This is the init() insanity. It is a crazy trick to make auto-complete of them work without having them actually exist on an instantiated object. We're removing them as we're adding methods/interfaces, they're useless with interface type hints anyway.

Just don't define protected $changed and should work.

I'm not sure how to review the hook_schema_0() changes because there are no docs for it, so somebody else should review that.
All other changes are identical to how it's handled for nodes. So that looks good.

I think this needs tests, but I'm not entirely certain … because not a single test verifies that Node::getChangedTime() is correct either. There is some test coverage for File::getChangedTime(), but it is very light.
This needs tests, see NodeSaveTest::testTimestamps().

I think this is RTBC-worthy, but somebody who's familiar with the entity/node system should probably RTBC it, in case there are special things that must be done.

I was wrong, updated #7, this needs tests for sure.

Thanks to @berdir for pointing out my mistake!

1. +++ b/core/modules/block/block.install
   +++ b/core/modules/block/block.install
   @@ -206,13 +206,15 @@ function block_update_8007() {
   
   @@ -206,13 +206,15 @@ function block_update_8007() {
        'info',
        'revision_id',
        'langcode',
   -    'type'
   +    'type',
   +    'changed',
      ));
      $revision_insert = db_insert('custom_block_revision')->fields(array(
        'id',
        'revision_id',
        'log',
   -    'info'
   +    'info',
   +    'changed',
      ));
      foreach ($results as $block) {
        $custom_block = array(
   @@ -221,13 +223,15 @@ function block_update_8007() {
   
   @@ -221,13 +223,15 @@ function block_update_8007() {
          'info' => $block->info,
          'revision_id' => $block->bid,
          'langcode' => Language::LANGCODE_NOT_SPECIFIED,
   -      'type' => 'basic'
   +      'type' => 'basic',
   +      'changed' => REQUEST_TIME,
        );
   

   This is updating an existing hook_update_N() function. Shouldn't we add a new hook_update_N() function for this, just like in #2074229: Add changed time tracking to taxonomy terms?

2. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockSaveTest.php
   @@ -84,6 +84,8 @@ public function testDeterminingChanges() {
   +    $this->assertTrue($block->getChangedTime(), 'Changed time maintained.');
   
   +++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/BlockUpgradePathTest.php
   @@ -60,6 +60,32 @@ public function testBlockUpgradeTitleLength() {
   +    $this->assertTrue($block->getChangedTime(), 'Changed time maintained.');
   

   This should be nonzero, not equal to boolean true. The used assert function sets the wrong expectations.

   Why not use assertNotEqual($block->getChangedTime(), 0), just like in #2074229: Add changed time tracking to taxonomy terms?

Thanks, that makes sense.

For final review, I noticed that the BlockUpgradePathTest hunk didn't make a whole lot sense. block.module provides config entity blocks. custom_block.module provides content entity blocks.

BlockUpgradePathTest only covers config entity blocks. And that's fine. But because there is no CustomBlockUpgradePathTest yet, Gábor was forced to inject additional *content* entity block (i.e. custom block) test coverage to BlockUpgradePathTest. That doesn't make a lot of sense.

Custom block upgrade path test coverage should probably be added in #1871700-73: Provide an upgrade path to the new Block architecture from Drupal 7, but it definitely does not belong here; it's just very confusing.

I also noticed that the added test coverage in CustomBlockTest were extremely sparse, just checking for "not equal to zero" is fine for the upgrade path, but not for actual test coverage. So I looked at NodeSaveTest for inspiration. I added test coverage analogous to what NodeSaveTest i.c.w. node_test.module/node_test_node_presave() does.

+1

Committed to 8.x.

.