Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The insert() / update() / delete() methods in FileField / FieldItem load each referenced file separately to update their usage.
The files for all items should be loaded in a single entity multiple load.
#2073661: Add a EntityReferenceField::referencedEntities() method would be helpful here.
Comment | File | Size | Author |
---|---|---|---|
#55 | file-usage-2073033-55.patch | 7.55 KB | claudiu.cristea |
#55 | interdiff.txt | 2.08 KB | claudiu.cristea |
#51 | file-usage-2073033-51.patch | 7.55 KB | claudiu.cristea |
#43 | file-usage-2073033-43.patch | 28.63 KB | claudiu.cristea |
#43 | interdiff.txt | 701 bytes | claudiu.cristea |
Comments
Comment #1
yched CreditAttribution: yched commentedPostponed on #2015697: Convert field type to typed data plugin for file and image modules
Comment #2
claudiu.cristea#2015697: Convert field type to typed data plugin for file and image modules is in so let's start the work on this.
Comment #3
claudiu.cristeaBut still needs #2073661: Add a EntityReferenceField::referencedEntities() method, right?
Comment #4
yched CreditAttribution: yched commentedUltimately this would use getreferencedEntities(), yes.
Meanwhile, the patch here could still add a temporary custom version in FileField and start refactoring the insert() / update() / delete() code - your call :-)
Comment #5
claudiu.cristeaJust a side-effect issue (and maybe a followup). I see that
file_usage()
function is just a wrapper aroundfile.usage
serviceWhy keeping that while we dropped all procedural wrappers like
config()
,lock()
, etc?Comment #6
yched CreditAttribution: yched commentedTrue, could probably be fixed here as well.
Comment #7
claudiu.cristeaThis patch is dropping also
file_usage()
wrapper (see #5 and #6).Comment #8
claudiu.cristeaForgot an unneeded variable inside the protected method.
Comment #10
claudiu.cristeaI can't find any reason why
Drupal\search\Tests\SearchCommentTest
Drupal\system\Tests\System\AccessDeniedTest
Drupal\system\Tests\System\PageNotFoundTest
... are failing. Locally they are passing.
Drupal\system\Tests\Upgrade\UserPictureUpgradePathTest
was fixed in this patch.Comment #11
claudiu.cristeaThere's no need to register the file.usage service. All we need is to access the container after
$this->performUpgrade()
has run.Comment #12
yched CreditAttribution: yched commentedRather getEntity() now (same in the other methods)
$files is keyed by $fid, right ?
Then maybe just foreach ($files as $fid => $file) { ? And then I'm not sure why we need to check for the fid not being NULL ?
Comment #13
claudiu.cristea@yched,
In #12.2:
Check your comment #117 from #2015697-117: Convert field type to typed data plugin for file and image modules. I removed the check here but still keep it few lines above where I had to add
array_filter()
.Comment #14
claudiu.cristeaAnd I think that this may be optimized more by adding
addMultiple()
anddeleteMultiple()
to\Drupal\file\FileUsage\FileUsageInterface
. Those new methods should accept a list of files as first argument and same other arguments. Then, inDatabaseFileUsageBackend
, those methods should use optimized SQL statements like multi-record INSERTs. That would replace$files
iterations and replace several SQLs with a single one.Comment #15
yched CreditAttribution: yched commentedTotally agreed on making FileUsageInterface multiple - feel like opening an issue for that ?
(if you ask me, it also seems a bit frustrating that the methods need a fully loaded $file object, when the job should mostly need an $fid, but I'm not really familiar enough with that area, maybe there are good reasons for that)
NULL fid : right, that happens in the current code, you can iterate on $items and find an $item with a NULL fid.
But this is "hidden" by the targetEntities() method now: the $ids array in there can contain a NULL, but there is no corresponding entity to load, so loadMultiple() will never return an array of entities with a NULL key.
It would probably be a good idea to run array_filter() on the list of $ids before calling loadMultiple(), though.
Also,
would probably be safer / faster.
Last nitpick : add a @todo on [#https://drupal.org/node/2073661] on the temporary targetEntities() ?
Other than those minor points, this looks RTBC to me.
Comment #16
claudiu.cristeaAdded #2092693: Make file usage faster as followup.
Just a comment on
targetEntities()
but mostly on #2073661: Add a EntityReferenceField::referencedEntities() method: The returned entity list WILL NOT keep the field delta mapping. We'll not be able to map between field items and entities unless the result will be keyed by field deltas. I think this is bad.Comment #17
claudiu.cristeaHere we go.
Comment #18
yched CreditAttribution: yched commentedOh, right, forgot about this "key by delta" thing. Yup, losing track of deltas is a bit unfortunate - even though returning entities by delta might make the code here a bit less straightforward ?
We could say that this is something for the more generic #2073661: Add a EntityReferenceField::referencedEntities() method to discuss, and meanwhile the code here is good for its specific scope ? Thus, temptatively setting to RTBC.
Comment #19
alexpottPatch no longer applies.
Comment #20
claudiu.cristeaAttached.
Comment #21
claudiu.cristea@yched, I just rerolled in #20, no changes. Can you re-RTBC? Thanks!
Comment #22
yched CreditAttribution: yched commentedYup.
Comment #23
claudiu.cristeaThis will fix also #2098655: Fatal error on article node deletion.
Added test for #2098655: Fatal error on article node deletion and marking this as critical. The "only-test" patch act also as interdiff.
Comment #24
yched CreditAttribution: yched commentedThe new test is nice, but I'm not sure it belongs to FileItemTest ? It is just a byproduct of this patch to also fix #2098655: Fatal error on article node deletion, but the specific bug that was pointed over there would still deserve its own standalone test, no ?
Comment #25
BerdirI suggested to place the test here, because that is the "Unit" test for the File Item field type and this can be tested using a unit test, so we should do it.
Comment #26
yched CreditAttribution: yched commentedOK. I don't know the specifics of the bug that got fixed, I had the impression it was more related to entity_ref generally, in which case FileItemTest would not be the best place to ensure no regressions. No biggie, feel free to ignore me on this.
Comment #27
BerdirThe bug was specifically about how FileItem implemented the file usage stuff, this snippet fixed it too: https://gist.github.com/Berdir/34299ecb260af6fec2e4
This implementation moves that code to FileItemList (after the rename), but it's still about the FileItem. It's not a general entity_ref thing, it works as designed.
Comment #28
yched CreditAttribution: yched commentedOh - another occurrence of #1988492: Avoid having empty field items by default then ? :-)
Thanks for the clarification. RTBC +1.
Comment #29
netsensei CreditAttribution: netsensei commentedI looked at this morning over travel back home. This definitely brings more sanity to who's responsible for performing file usage operations.
Also fixes #2098655: Fatal error on article node deletion I was thinking along the same lines: either by checking against $entity or, fiddling with $this->get('entity')->getTarget(). Both didn't feel right. Something like a referencedEntities method also came to mind. :-)
RTBC +1!
Comment #31
claudiu.cristea#23: file-usage-2073033-23.patch queued for re-testing.
Comment #32
claudiu.cristeaQueued for retesting as we had issues with the bot. That bot has tested and then re-tested the same patch for 6 hours!
Comment #33
claudiu.cristea@yched, please re-RTBC. Tests passed.
Comment #34
BerdirYes, back to RTBC.
Comment #35
claudiu.cristea#2086893: Can't delete content: Call to a member function id() on a non-object in DatabaseFileUsageBackend.php reports also the bug proved by the "test-only" patch from #23 and was marked as duplicate of this.
Comment #36
claudiu.cristeaChanged the status by mistake. Sorry!
Comment #37
alexpottPatch no longer applies.
Comment #38
netsensei CreditAttribution: netsensei commentedcore/modules/file/lib/Drupal/file/Plugin/field/field_type/FileField.php was renamed to FileFieldItemList.php in #2051923: Rename \Drupal\Core\Entity\Field\Field, related subclasses and interfaces to *FieldItemList
I've rerolled the patch.
Comment #39
claudiu.cristeaA doc comment was also left out in #2051923: Rename \Drupal\Core\Entity\Field\Field, related subclasses and interfaces to *FieldItemList.
Comment #40
BerdirThis should have an isset($this->list), as there are currently some cases that set it to NULL (rest.module related). See how ItemList.php does it.
Comment #41
claudiu.cristeaNULLs are stripped out 2 lines after in
array_filter()
.Comment #42
claudiu.cristeaUntagging.
Comment #43
claudiu.cristeaSorry, I didn't read carefully #40, it was about
$this->list
not about$item->target_id
. Here's the change.Comment #44
BerdirYes, that. Might need to be handled in the other issue about referencedEntities() too?
Comment #45
claudiu.cristeaBetter fix this where it is generated (rest.module?) to avoid tons of "if isset()". Ensure at least an empty array to ItemList->list :)
Comment #46
BerdirWe've been discussing this today, that's why I checked. It's tricky. #1988492: Avoid having empty field items by default is about that too.
Comment #47
catchGenerally we've been @deprecating one-line wrappers rather than removing them - so we can do that in one sweep later. Any reason to ditch it here?
Also this adds a lot of Drupal:service() calls - is it worth adding a method?
Comment #48
alexpottThis issue seems to be doing two things which do not have to be done together.
Can we do the first one here and then open a follow up do discuss the second. The second part of this is not critical.
Smaller patches are easier to review.
Comment #49
claudiu.cristea@catch, @alexpott please check #5 and #6.
Comment #50
BerdirWe can't add a method for this to Drupal, as it's not a core service, it's provided by file.module. And don't want to delay this too on the "how should Drupal-for-$module classes by named" discussion, which we forgot to talk about in Prague.
Agree with file_usage() removal being off-topic here.
Comment #51
claudiu.cristeaOK. Splited #2104229: Deprecate file_usage().
Here's the patch only with "optimize" issue but this is fixing also o bug in HEAD. So please provide a review or RTBC asap. Thanks!
Comment #52
BerdirYay for a 8kb patch instead of 26kb :)
Comment #53
catch#51: file-usage-2073033-51.patch queued for re-testing.
Comment #54
Wim LeersSorry, a bunch of nitpicks:
Reads strangely to me. "newly uploaded files" maybe?
s/ids/IDs/ to be consistent with the comments elsewhere.
s/to field/to the field/ ?
Omit "count", or add "count" elsewhere.
Comment #55
claudiu.cristea@Wim Leers, thank you!
Comment #56
BerdirChanges look good to me, let's try again.
Comment #57
catchCommitted/pushed to 8.x, thanks!
Comment #58.0
(not verified) CreditAttribution: commentedLink to #2073661: Add a EntityReferenceField::referencedEntities() method