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.
Related Issues
#2044583: Add EntityChangedInterface to allow entities with "changed" field to be properly cached
#2074191: [META] Add changed timestamp tracking to content entities
Comment | File | Size | Author |
---|---|---|---|
#16 | interdiff.txt | 434 bytes | benjy |
#16 | 2074475_16.patch | 21.2 KB | benjy |
#14 | interdiff.txt | 3.42 KB | benjy |
#14 | 2074475_14.patch | 21.32 KB | benjy |
#12 | renamed-file-managed-timestamp-2074475-12.patch | 21.73 KB | Gábor Hojtsy |
Comments
Comment #1
Dave ReidComment #2
semei CreditAttribution: semei commentedWill there be a backport of this fix for D7?
Comment #3
Dave ReidWe really need this before beta since it affects schema.
Comment #4
Dave ReidComment #5
catchThis 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().
Comment #6
benjy CreditAttribution: benjy commentedHere's a first attempt, let's see what the test bot thinks.
Comment #8
benjy CreditAttribution: benjy commentedFixed 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.
Comment #9
Berdir#2145103: Provide non-configurable field types for entity created, changed and timestamp fields would bring standardization for that field.
Comment #10
Berdir8: renamed-file-managed-timestamp-2074475-8.patch queued for re-testing.
Comment #12
Gábor HojtsyRerolled since this failed to apply at a few places now.
Comment #13
BerdirThat 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.
AFAIK We still need an update hook to make sure we won't forget it when doing the migration, just no test coverage.
Lies! (the description :))
debug left-over?
This should probably also set the created timestamp.
Comment #14
benjy CreditAttribution: benjy commentedDone.
Comment #15
Gábor HojtsyDoing 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:
Also "added created property", so something like "Renamed 'timestamp' to 'changed', added 'created'." (using quotes since "added created" sounds like broken English :D)
Comment #16
benjy CreditAttribution: benjy commentedThe 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.
Comment #17
Gábor HojtsyAll right, looks good to me :) Looks like @Berdir did not find any other issue either.
Comment #18
catchNice to see this one green. Committed/pushed to 8.x, thanks!
Could use a short change notice.
Comment #19
Gábor HojtsyCreated https://drupal.org/node/2168561 as a change notice.
Comment #21
Gábor HojtsyFix media tag.