Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

claudiu.cristea’s picture

Assigned: Unassigned » claudiu.cristea
Status: Postponed » Needs work
claudiu.cristea’s picture

Status: Needs work » Postponed
yched’s picture

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

claudiu.cristea’s picture

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?

yched’s picture

True, could probably be fixed here as well.

claudiu.cristea’s picture

Title: Optimize file_usage updates in file / image fields with entity multi-load » Optimize file usage updates in file/image fields. Drop file_usage()
Status: Postponed » Needs review
FileSize
27.38 KB

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

claudiu.cristea’s picture

FileSize
677 bytes
27.38 KB

Forgot an unneeded variable inside the protected method.

Status: Needs review » Needs work

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

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
1.2 KB
27.73 KB

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.

claudiu.cristea’s picture

FileSize
756 bytes
27.37 KB

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

yched’s picture

  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 ?

claudiu.cristea’s picture

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

claudiu.cristea’s picture

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.

yched’s picture

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.

claudiu.cristea’s picture

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.

claudiu.cristea’s picture

FileSize
995 bytes
27.56 KB

Here we go.

yched’s picture

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.

alexpott’s picture

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

Patch no longer applies.

claudiu.cristea’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
27.55 KB

Attached.

claudiu.cristea’s picture

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

yched’s picture

Status: Needs review » Reviewed & tested by the community

Yup.

claudiu.cristea’s picture

Priority: Normal » Critical
FileSize
28.31 KB
722 bytes

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.

yched’s picture

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 ?

Berdir’s picture

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.

yched’s picture

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.

Berdir’s picture

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.

yched’s picture

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

netsensei’s picture

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.

claudiu.cristea’s picture

Status: Needs work » Needs review

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

claudiu.cristea’s picture

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

claudiu.cristea’s picture

@yched, please re-RTBC. Tests passed.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Yes, back to RTBC.

claudiu.cristea’s picture

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.

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community

Changed the status by mistake. Sorry!

alexpott’s picture

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

Patch no longer applies.

netsensei’s picture

Status: Needs work » Needs review
FileSize
27.14 KB

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.

claudiu.cristea’s picture

Berdir’s picture

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.

claudiu.cristea’s picture

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

claudiu.cristea’s picture

Issue tags: -Needs reroll

Untagging.

claudiu.cristea’s picture

FileSize
701 bytes
28.63 KB

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

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

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

claudiu.cristea’s picture

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

Berdir’s picture

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.

catch’s picture

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?

alexpott’s picture

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.

claudiu.cristea’s picture

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

Berdir’s picture

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.

claudiu.cristea’s picture

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
FileSize
7.55 KB

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!

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

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

Wim Leers’s picture

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.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
2.08 KB
7.55 KB

@Wim Leers, thank you!

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

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.

Anonymous’s picture