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.

Files: 
CommentFileSizeAuthor
#55 file-usage-2073033-55.patch7.55 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 58,866 pass(es).
[ View ]
#55 interdiff.txt2.08 KBclaudiu.cristea
#51 file-usage-2073033-51.patch7.55 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 58,956 pass(es).
[ View ]
#43 file-usage-2073033-43.patch28.63 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 59,115 pass(es).
[ View ]
#43 interdiff.txt701 bytesclaudiu.cristea
#39 file-usage-2073033-39.patch28.56 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#39 interdiff.txt595 bytesclaudiu.cristea
#38 file-usage-2073033-38.patch27.14 KBnetsensei
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#23 file-usage-2073033-23-FAIL.patch722 bytesclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] 58,306 pass(es), 110 fail(s), and 26 exception(s).
[ View ]
#23 file-usage-2073033-23.patch28.31 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 58,644 pass(es).
[ View ]
#20 file-usage-2073033-20.patch27.55 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 58,638 pass(es).
[ View ]
#17 file-usage-2073033-17.patch27.56 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 59,666 pass(es).
[ View ]
#17 interdiff.txt995 bytesclaudiu.cristea
#13 file-usage-2073033-13.patch27.4 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 59,581 pass(es).
[ View ]
#13 interdiff.txt1.86 KBclaudiu.cristea
#11 file-usage-2073033-11.patch27.37 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 59,585 pass(es).
[ View ]
#11 interdiff.txt756 bytesclaudiu.cristea
#10 file-usage-2073033-10.patch27.73 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 59,184 pass(es).
[ View ]
#10 interdiff.txt1.2 KBclaudiu.cristea
#8 file-usage-2073033-9.patch27.38 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] 58,779 pass(es), 6 fail(s), and 3 exception(s).
[ View ]
#8 interdiff.txt677 bytesclaudiu.cristea
#7 file-usage-2073033-7.patch27.38 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Comments

Assigned:Unassigned» claudiu.cristea
Status:Postponed» Needs work

Status:Needs work» Postponed

Ultimately 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 :-)

Just a side-effect issue (and maybe a followup). I see that file_usage() function is just a wrapper around file.usage service

function file_usage() {
  return \Drupal::service('file.usage');
}

Why keeping that while we dropped all procedural wrappers like config(), lock(), etc?

True, could probably be fixed here as well.

Title:Optimize file_usage updates in file / image fields with entity multi-loadOptimize file usage updates in file/image fields. Drop file_usage()
Status:Postponed» Needs review
StatusFileSize
new27.38 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

This patch is dropping also file_usage() wrapper (see #5 and #6).

StatusFileSize
new677 bytes
new27.38 KB
FAILED: [[SimpleTest]]: [MySQL] 58,779 pass(es), 6 fail(s), and 3 exception(s).
[ View ]

Forgot an unneeded variable inside the protected method.

Status:Needs review» Needs work

The last submitted patch, file-usage-2073033-9.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.2 KB
new27.73 KB
PASSED: [[SimpleTest]]: [MySQL] 59,184 pass(es).
[ View ]

I 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.

StatusFileSize
new756 bytes
new27.37 KB
PASSED: [[SimpleTest]]: [MySQL] 59,585 pass(es).
[ View ]

There's no need to register the file.usage service. All we need is to access the container after $this->performUpgrade() has run.

  1. +++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileField.php
    @@ -22,50 +22,97 @@ public function defaultValuesForm(array &$form, array &$form_state) { }
    +    $entity = $this->getRoot();

    Rather getEntity() now (same in the other methods)

  2. +++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileField.php
    @@ -22,50 +22,97 @@ public function defaultValuesForm(array &$form, array &$form_state) { }
    +    foreach ($files as $file) {
    +      if ($file->fid && !in_array($file->fid, $original_fids)) {

    $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 ?

StatusFileSize
new1.86 KB
new27.4 KB
PASSED: [[SimpleTest]]: [MySQL] 59,581 pass(es).
[ View ]

@yched,

In #12.2:

And then I'm not sure why we need to check for the fid not being NULL ?

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().

And I think that this may be optimized more by adding addMultiple() and deleteMultiple() to \Drupal\file\FileUsage\FileUsageInterface. Those new methods should accept a list of files as first argument and same other arguments. Then, in DatabaseFileUsageBackend, those methods should use optimized SQL statements like multi-record INSERTs. That would replace $files iterations and replace several SQLs with a single one.

Totally 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,

if ($ids) {
  return loadMultiple($ids);
}
return array();

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.

Added #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.

StatusFileSize
new995 bytes
new27.56 KB
PASSED: [[SimpleTest]]: [MySQL] 59,666 pass(es).
[ View ]

Here we go.

Status:Needs review» Reviewed & tested by the community

Oh, 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.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new27.55 KB
PASSED: [[SimpleTest]]: [MySQL] 58,638 pass(es).
[ View ]

Attached.

@yched, I just rerolled in #20, no changes. Can you re-RTBC? Thanks!

Status:Needs review» Reviewed & tested by the community

Yup.

Priority:Normal» Critical
StatusFileSize
new28.31 KB
PASSED: [[SimpleTest]]: [MySQL] 58,644 pass(es).
[ View ]
new722 bytes
FAILED: [[SimpleTest]]: [MySQL] 58,306 pass(es), 110 fail(s), and 26 exception(s).
[ View ]

This 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.

The 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 ?

I 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.

OK. 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.

The 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.

Oh - another occurrence of #1988492: Avoid having empty field items by default then ? :-)
Thanks for the clarification. RTBC +1.

I 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!

Status:Reviewed & tested by the community» Needs work

The last submitted patch, file-usage-2073033-23.patch, failed testing.

Status:Needs work» Needs review

#23: file-usage-2073033-23.patch queued for re-testing.

Queued for retesting as we had issues with the bot. That bot has tested and then re-tested the same patch for 6 hours!

@yched, please re-RTBC. Tests passed.

Status:Needs review» Reviewed & tested by the community

Yes, back to RTBC.

Status:Reviewed & tested by the community» Needs review

#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.

Status:Needs review» Reviewed & tested by the community

Changed the status by mistake. Sorry!

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

Status:Needs work» Needs review
StatusFileSize
new27.14 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

core/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.

Issue tags:-Needs reroll
StatusFileSize
new595 bytes
new28.56 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Status:Needs review» Needs work
Issue tags:+Needs reroll

+++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileFieldItemList.php
@@ -22,50 +22,102 @@ public function defaultValuesForm(array &$form, array &$form_state) { }
+    foreach ($this->list as $item) {
+      $ids[] = $item->target_id;
+    }

This 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.

Status:Needs work» Needs review

This 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.

NULLs are stripped out 2 lines after in array_filter().

Issue tags:-Needs reroll

Untagging.

StatusFileSize
new701 bytes
new28.63 KB
PASSED: [[SimpleTest]]: [MySQL] 59,115 pass(es).
[ View ]

Sorry, I didn't read carefully #40, it was about $this->list not about $item->target_id. Here's the change.

Status:Needs review» Reviewed & tested by the community

Yes, that. Might need to be handled in the other issue about referencedEntities() too?

Better fix this where it is generated (rest.module?) to avoid tons of "if isset()". Ensure at least an empty array to ItemList->list :)

We'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.

Generally 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?

Status:Reviewed & tested by the community» Needs work

This issue seems to be doing two things which do not have to be done together.

  • Optimize file usage updates in file/image fields
  • Drop file_usage()

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.

@catch, @alexpott please check #5 and #6.

We 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.

Title:Optimize file usage updates in file/image fields. Drop file_usage()Optimize file usage updates in file/image fields
Status:Needs work» Needs review
StatusFileSize
new7.55 KB
PASSED: [[SimpleTest]]: [MySQL] 58,956 pass(es).
[ View ]

OK. 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!

Status:Needs review» Reviewed & tested by the community

Yay for a 8kb patch instead of 26kb :)

#51: file-usage-2073033-51.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work

Sorry, a bunch of nitpicks:

  1. +++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileFieldItemList.php
    @@ -22,50 +22,105 @@ public function defaultValuesForm(array &$form, array &$form_state) { }
    +    // Add a new usage for new uploaded files.

    Reads strangely to me. "newly uploaded files" maybe?

  2. +++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileFieldItemList.php
    @@ -22,50 +22,105 @@ public function defaultValuesForm(array &$form, array &$form_state) { }
    +    // Get current target file entities and file ids.
    ...
    +   *   An array with the list of target file entities keyed by file id.

    s/ids/IDs/ to be consistent with the comments elsewhere.

  3. +++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileFieldItemList.php
    @@ -22,50 +22,105 @@ public function defaultValuesForm(array &$form, array &$form_state) { }
    +    // Get the file IDs attached to field before this update.

    s/to field/to the field/ ?

  4. +++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileFieldItemList.php
    @@ -22,50 +22,105 @@ public function defaultValuesForm(array &$form, array &$form_state) { }
    +    // Decrement the file usage count by 1.

    Omit "count", or add "count" elsewhere.

Status:Needs work» Needs review
StatusFileSize
new2.08 KB
new7.55 KB
PASSED: [[SimpleTest]]: [MySQL] 58,866 pass(es).
[ View ]

@Wim Leers, thank you!

Status:Needs review» Reviewed & tested by the community

Changes look good to me, let's try again.

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

Status:Fixed» Closed (fixed)

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