Problem/Motivation

Files don't have created time tracking and don't use the standard name for the changed timestamp property. See various reasons why this is a problem at #2074191: [META] Add changed timestamp tracking to content entities.

Proposed resolution

Add created time tracking to files and fix the changed timestamp name.

Remaining tasks

Needs upgrade path. How do we provide created time values? Just use zero? Loop through each file record and use min(filectime($file->uri), $file->timestamp)?

User interface changes

None.

API changes

Schema changed. Would need to add a getCreatedTime method? 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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Component: user.module » file.module
semei’s picture

Will there be a backport of this fix for D7?

Dave Reid’s picture

Priority: Normal » Major
Issue summary: View changes
Issue tags: +beta blocker

We really need this before beta since it affects schema.

Dave Reid’s picture

Issue summary: View changes
catch’s picture

Priority: Major » Critical

This needs adding for migrate iirc as well as anything else.

Bumping to critical since it's always good to avoid writing a hook_update_N().

benjy’s picture

Status: Active » Needs review
FileSize
10.48 KB

Here's a first attempt, let's see what the test bot thinks.

Status: Needs review » Needs work

The last submitted patch, 6: renamed-file-managed-timestamp-2074475-6.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
11.73 KB
21.73 KB

Fixed up the tests, added getCreatedTime() to the FileInterface, changed $this->changed to $this->changed->value in Entity/File::preSave() and added the yml for views by copying the changed version for created.

Just a note it seems the created/changed timestamps logic in preSave() is different across Entity/File.php, Entity/Node.php and Entity/Comment.php. Probably worth standardising how we handle entities with these timestamps at some point.

Berdir’s picture

Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, 8: renamed-file-managed-timestamp-2074475-8.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
21.73 KB

Rerolled since this failed to apply at a few places now.

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/file/file.api.php
    @@ -82,7 +82,7 @@ function hook_file_validate(Drupal\file\FileInterface $file) {
     }
    ...
     function hook_file_presave(Drupal\file\FileInterface $file) {
       // Change the file timestamp to an hour prior.
    -  $file->timestamp -= 3600;
    +  $file->changed -= 3600;
    

    That example is invalid, this won't work as $file->changed is an object (as was timestamp). $file->changed->value -= 3600 would work.

    However, maybe it's better to use a completely different property, changing the changed timestamp seems kind of an edge case to use as an example? There's not much could possibly be done here, though.

  2. +++ b/core/modules/file/file.install
    @@ -78,8 +78,15 @@ function file_schema() {
           ),
    -      'timestamp' => array(
    -        'description' => 'UNIX timestamp for when the file was added.',
    +      'created' => array(
    +        'description' => 'UNIX timestamp for when the file added.',
    +        'type' => 'int',
    +        'unsigned' => TRUE,
    +        'not null' => TRUE,
    +        'default' => 0,
    +      ),
    +      'changed' => array(
    +        'description' => 'UNIX timestamp for when the file was last changed.',
    

    AFAIK We still need an update hook to make sure we won't forget it when doing the migration, just no test coverage.

  3. +++ b/core/modules/file/lib/Drupal/file/Entity/File.php
    @@ -253,10 +264,14 @@ public static function baseFieldDefinitions($entity_type) {
         return $fields;
    ...
     
    -    $fields['timestamp'] = FieldDefinition::create('integer')
    +    $fields['created'] = FieldDefinition::create('integer')
           ->setLabel(t('Created'))
           ->setDescription(t('The time that the node was created.'));
     
    +    $fields['changed'] = FieldDefinition::create('integer')
    +      ->setLabel(t('Changed'))
    +      ->setDescription(t('The time that the node was last changed.'));
    +
    

    Lies! (the description :))

  4. +++ b/core/modules/file/lib/Drupal/file/Tests/FileTokenReplaceTest.php
    @@ -74,6 +77,7 @@ function testFileTokenReplacement() {
         }
    
    @@ -53,8 +53,10 @@ function testFileTokenReplacement() {
     
    @@ -63,6 +65,7 @@ function testFileTokenReplacement() {
    
    @@ -63,6 +65,7 @@ function testFileTokenReplacement() {
     
         foreach ($tests as $input => $expected) {
           $output = $token_service->replace($input, array('file' => $file), array('langcode' => $language_interface->id));
    +      $this->verbose(print_r(array($output, $expected), 1));
           $this->assertEqual($output, $expected, format_string('Sanitized file token %token replaced.', array('%token' => $input)));
         }
     
    @@ -74,6 +77,7 @@ function testFileTokenReplacement() {
    
    @@ -74,6 +77,7 @@ function testFileTokenReplacement() {
     
         foreach ($tests as $input => $expected) {
           $output = $token_service->replace($input, array('file' => $file), array('langcode' => $language_interface->id, 'sanitize' => FALSE));
    +      $this->verbose(print_r(array($output, $expected), 1));
           $this->assertEqual($output, $expected, format_string('Unsanitized file token %token replaced.', array('%token' => $input)));
    

    debug left-over?

  5. +++ b/core/modules/user/user.install
    @@ -555,7 +555,7 @@ function user_update_8011() {
           ))
    
    +++ b/core/modules/user/user.install
    @@ -555,7 +555,7 @@ function user_update_8011() {
    
    @@ -555,7 +555,7 @@ function user_update_8011() {
             'langcode' => Language::LANGCODE_NOT_SPECIFIED,
             'filesize' => filesize($destination),
             'filemime' => file_get_mimetype($destination),
    -        'timestamp' => REQUEST_TIME,
    +        'changed' => REQUEST_TIME,
    

    This should probably also set the created timestamp.

benjy’s picture

Status: Needs work » Needs review
FileSize
21.32 KB
3.42 KB

Done.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Doing upgrade stubs like this sounds strange but ok. There is not much point in writing it, since it will not run... Only one minor thing left AFAIS:

+++ b/core/modules/file/file.install
@@ -321,3 +328,9 @@ function file_update_8004() {
+/**
+ * Renamed timestamp to changed.
+ */
+function file_update_8005() {
+  // This is a stub to help with migrations.
+}

Also "added created property", so something like "Renamed 'timestamp' to 'changed', added 'created'." (using quotes since "added created" sounds like broken English :D)

benjy’s picture

Status: Needs work » Needs review
FileSize
21.2 KB
434 bytes

The reason I never mentioned "created" was because the stub was only to indicate that when doing a migration from D6/7 to D8 that timestamp should be mapped to created.

I spoke with alexpott and he suggested we remove it and just put the details in a change notice which makes sense to me.

Also note this issue, #2168011: Remove all 7.x to 8.x update hooks and disallow updates from the previous major version which is going to remove all 7.x updates.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

All right, looks good to me :) Looks like @Berdir did not find any other issue either.

catch’s picture

Title: Rename {file_managed}.timestamp to 'changed' and add a created timestamp » Change notice: Rename {file_managed}.timestamp to 'changed' and add a created timestamp
Priority: Critical » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Nice to see this one green. Committed/pushed to 8.x, thanks!

Could use a short change notice.

Gábor Hojtsy’s picture

Title: Change notice: Rename {file_managed}.timestamp to 'changed' and add a created timestamp » Rename {file_managed}.timestamp to 'changed' and add a created timestamp
Priority: Major » Critical
Status: Active » Fixed
Issue tags: -Needs change record

Created https://drupal.org/node/2168561 as a change notice.

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Issue tags: -Media Initiative +D8Media

Fix media tag.