Problem/Motivation
If you load a file entity through entity manager $file->status is of "FieldItemList" type.
The condition in "_editor_record_file_usage" function is :
if ($file->status !== FILE_STATUS_PERMANENT)
As type of FILE_STATUS_PERMANENT is "int" we have a wrong assumption that file is not permanent.
Field values should be accessed as $entity->field_name->value
Proposed resolution
Use available API instead of status property(e.g. $file->isPermanent()).
Remaining tasks
User interface changes
N/A
API changes
Data model changes
N/A
Release notes snippet
TBC(?)
| Comment | File | Size | Author |
|---|---|---|---|
| #28 | 2923428-28.patch | 3.72 KB | manuel garcia |
| #28 | interdiff-2923428-26-28.txt | 4.17 KB | manuel garcia |
| #26 | 2923428-26.patch | 5.06 KB | manuel garcia |
| #26 | interdiff-2923428-24-26.txt | 927 bytes | manuel garcia |
| #24 | 2923428-24.patch | 4.7 KB | manuel garcia |
Comments
Comment #2
undertext commentedComment #3
undertext commentedComment #4
undertext commentedComment #5
berdirSurprised there is no existing issue about this, did you check?
As simple as the fix might be, the fact that it exists means that we need test coverage for this.
Comment #6
undertext commentedI went through issues filtered by "editor.module" component and didn't find similar issue.
Maybe this issue is unnoticed just because "file" entity is not revisionable.
In my case I use "multiversion" module which forces "file" entity to be revisionable, so i get a new file revision after each node save.
Will try to add tests shortly.
Comment #7
undertext commentedAdding test-only patch.
Comment #9
undertext commentedUpdating test-only patch.
Comment #10
undertext commentedComment #12
undertext commentedAnd now the real patch with test included, this one should be success.
Comment #13
douggreen commentedThe solution looks good. I also confirmed that we have no other usages of
== FILE_STATUS_or!= FILE_STATUS_in core.Comment #14
douggreen commentedbump version
Comment #16
manuel garcia commentedComment #17
manuel garcia commentedQueued #12 to be tested against 8.6.x
Comment #19
andypostFixed inline with changes in #3021833-12: Move FILE_STATUS_PERMANENT to \Drupal\file\FileInterface
Looks test should be extended to make sure that status really set
Comment #22
badjava commentedRerolled the patch for 8.7.x.
Comment #23
manuel garcia commentedComment #24
manuel garcia commentedAddressing my own review here, mainly cleaning up.
Deprecated, lets use
$this->entityTypeManagerinstead.This should be just
$node = Node::create([If I'm reading this class correctly, the save count for the given entity is only available if it's been saved before, so we should default to returning
0if$this->saveCount[$entity->id()]isn't set.Comment #26
manuel garcia commentedRe #19:
Good point @andypost, adding coverage for that here.
Comment #27
amateescu commentedI think it would be better if we would record the number of saves with a
hook_ENTITY_TYPE_presave()implementation rather than using a special entity storage handler.Comment #28
manuel garcia commentedGood idea @amateescu - that way this implementation is a lot simpler - thanks!
Using state to keep track of the number of times a file is saved here.
Note that I went for using the file name as the array key of the state value because the file id is not yet available when it's a new file. Opened to using something else if anyone has a better idea :)
Comment #29
vijaycs85Updating issue summary to reflect the solution proposed in the latest patch.
Comment #30
amateescu commentedThis looks much better now!
Comment #31
alexpottCommitted and pushed aeefe0664b to 8.8.x and f3d837af83 to 8.7.x. Thanks!
Credited @amateescu and @douggreen for reviews.
Comment #32
alexpott