Problem/Motivation

Custom blocks don't have changed time tracking. See various reasons why this is a problem at #2074191: [META] Add changed timestamp tracking to content entities.

Proposed resolution

Add change time tracking to custom blocks.

Remaining tasks

User interface changes

None.

API changes

Schema changed. New getChangedTime method added. This naming is already used on other entity types. See #2044583: Add EntityChangedInterface to allow entities with "changed" field to be properly cached for unification of this.

#2044583: Add EntityChangedInterface to allow entities with "changed" field to be properly cached
#2074191: [META] Add changed timestamp tracking to content entities

Files: 
CommentFileSizeAuthor
#16 changed-custom-block-16.patch7.95 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 58,512 pass(es).
[ View ]
#16 interdiff.txt4.01 KBWim Leers
#15 changed-custom-block-15.patch7.48 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 58,011 pass(es).
[ View ]
#13 changed-custom-block-13.patch7.47 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 58,020 pass(es).
[ View ]
#13 interdiff.txt1.04 KBGábor Hojtsy
#12 interdiff.txt3.34 KBGábor Hojtsy
#12 changed-custom-block-12.patch7.34 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 58,519 pass(es).
[ View ]
#6 changed-custom-block-6.patch4 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 58,557 pass(es).
[ View ]
#3 changed-custom-block-2.patch4.34 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 58,485 pass(es), 1 fail(s), and 55 exception(s).
[ View ]
changed-custom-block.patch4.03 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 58,385 pass(es), 1 fail(s), and 55 exception(s).
[ View ]

Comments

Status:Needs review» Needs work

The last submitted patch, changed-custom-block.patch, failed testing.

As per @berdir as of yesterday, we should now have a baseFieldDefinitions() entry as well for this. Let's see how this works :)

Status:Needs work» Needs review
StatusFileSize
new4.34 KB
FAILED: [[SimpleTest]]: [MySQL] 58,485 pass(es), 1 fail(s), and 55 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, changed-custom-block-2.patch, failed testing.

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.

Status:Needs work» Needs review
StatusFileSize
new4 KB
PASSED: [[SimpleTest]]: [MySQL] 58,557 pass(es).
[ View ]

Yeah I previously tried to have the unset() in init() which I thought would solve the problem (I guess based on your explanation) but that did not work either. Let's see not defining the property then :) Its a magic property either way.

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.

Issue tags:+Needs tests

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

Thanks to @berdir for pointing out my mistake!

Issue tags:-Needs tests

hook_schema_0() is executed from core/lib/Drupal/Core/Extension/UpdateModuleHandler.php with this code:

<?php
   
foreach ($module_list as $module) {
     
// Check for initial schema and install it. The schema version of a newly
      // installed module is always 0. Using 8000 here would be inconsistent
      // since $module_update_8000() may involve a schema change, and we want
      // to install the schema as it was before any updates were added.
     
module_load_install($module);
     
$function = $module . '_schema_0';
      if (
function_exists($function)) {
       
$schema = $function();
        foreach (
$schema as $table => $spec) {
         
db_create_table($table, $spec);
        }
      }
?>

Views is the only other module that has such a schema. This is applicable to new modules that need to create schemas as part of being installed in an update. See #1929816: Remove all references to/implementations of hook_schema_0() especially comment #2 there.

Issue tags:+Needs tests

Issue tags:-Needs tests

In, #2074229: Add changed time tracking to taxonomy terms, we figured out this needs to be added to the views integration of the module. Custom blocks don't have views integration for some reason. Instead of adding it all with the changed field, I think this is out of scope for this patch so opened #2077833: Add views integration to custom_block module.

StatusFileSize
new7.34 KB
PASSED: [[SimpleTest]]: [MySQL] 58,519 pass(es).
[ View ]
new3.34 KB

All right, provided test for creating new blocks as well as same after upgrade path. Also added code to upgrade changed time for existing blocks properly in the upgrade path. Anything else missing?

StatusFileSize
new1.04 KB
new7.47 KB
PASSED: [[SimpleTest]]: [MySQL] 58,020 pass(es).
[ View ]

After #2044583: Add EntityChangedInterface to allow entities with "changed" field to be properly cached landed, we should use the EntityChangedInterface here. I think this is complete. Any concerns?

Status:Needs review» Needs work
  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?

Status:Needs work» Needs review
StatusFileSize
new7.48 KB
PASSED: [[SimpleTest]]: [MySQL] 58,011 pass(es).
[ View ]

@WimLeers: we don't yet maintain a head 2 head upgrade path so updating existing update functions is fine, when the code is already there. That was not the case with the taxonomy patch.

I am fine with NotEqual checking as well. Updated patch attached with only that change.

StatusFileSize
new4.01 KB
new7.95 KB
PASSED: [[SimpleTest]]: [MySQL] 58,512 pass(es).
[ View ]

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.

I think its overkill to add so much test coverage for each column added, but if that makes people happy, so be it.

Status:Needs review» Reviewed & tested by the community

Anyway, the test coverage looks much better than mine. I think this should be ready to go.

+1

Status:Reviewed & tested by the community» Fixed

Committed to 8.x.

Issue tags:-sprint

.

Automatically closed -- issue fixed for 2 weeks with no activity.