Part of meta-issue #2225347: [META] Replace calls to ENTITY_TYPE_load() and ENTITY_TYPE_load_multiple() with static method calls

See the detailed explanations and examples there.

Allowed in beta evaluation is on the meta. Allowed as a prioritized change: removing deprecated code.

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions

User interface changes

No.

API changes

?

CommentFileSizeAuthor
#133 replace_all_instances-2321969-133.patch45.3 KBJeroenT
#133 interdiff.txt1.49 KBJeroenT
#131 interdiff.txt1.17 KBMile23
#131 2321969_131.patch46.85 KBMile23
#130 2321969_130.patch46.83 KBMile23
#124 interdiff-119-124.txt57 KBrosinegrean
#124 replace_all_instances-2321969-124.patch46.84 KBrosinegrean
#119 replace_all_instances-2321969-119.patch57.31 KBJeroenT
#116 interdiff.txt572 bytesJeroenT
#116 replace_all_instances-2321969-116.patch59.85 KBJeroenT
#114 replace_all_instances-2321969-114.patch59.94 KBJeroenT
#112 replace_all_instances-2321969-111.patch59.84 KBJeroenT
#110 interdiff.txt973 bytesSumi
#110 drupal_2321969_110.patch59.02 KBSumi
#108 2321969-108.patch59.02 KBrpayanm
#106 interdiff.txt1.63 KBLinL
#105 interdiff.txt560 bytesLinL
#105 2321969-105.patch58.99 KBLinL
#104 interdiff.txt560 bytesLinL
#104 2321969-104.patch59.61 KBLinL
#103 interdiff.txt686 bytesLinL
#103 2321969-103.patch59.68 KBLinL
#101 2321969-101.patch59.7 KBrpayanm
#101 2321969-interdiff.txt1.15 KBrpayanm
#100 replace_all_instances-2321969-100.patch60.85 KBrosinegrean
#95 2321969-95.patch65.92 KBrpayanm
#91 interdiff-2321969-87-91.txt1.15 KBsubhojit777
#91 replace_all_instances-2321969-91.patch66.08 KBsubhojit777
#87 replace_all_instances-2321969-87.patch60.13 KBsubhojit777
#87 interdiff-2321969-83-87.txt1.5 KBsubhojit777
#83 replace_all_instances-2321969-83.patch60.32 KBsubhojit777
#83 interdiff-2321969-81-83.txt1.77 KBsubhojit777
#81 replace_all_instances-2321969-81.patch60.4 KBsubhojit777
#81 interdiff-2321969-76-81.txt789 bytessubhojit777
#76 replace_all_instances-2321969-76.patch60.76 KBsubhojit777
#76 interdiff-2321969-73-76.txt5.78 KBsubhojit777
#73 replace_all_instances-2321969-73.patch63.19 KBsubhojit777
#73 interdiff-2321969-70-73.txt1.94 KBsubhojit777
#70 replace_all_instances-2321969-70.patch62.41 KBsubhojit777
#70 interdiff-2321969-67-70.txt872 bytessubhojit777
#67 replace_all_instances-2321969-67.patch62.4 KBsubhojit777
#63 2321969-rerolled-63.patch65.32 KBprashantgoel
#59 2321969-rerolled-59.patch65.33 KBprashantgoel
#56 2321969-rerolled-56.patch65.11 KBprashantgoel
#55 2321969-55.patch65.26 KBLinL
#54 2321969-54.patch65.26 KBLinL
#53 2321969-53.patch65 KBpcambra
#53 interdiff.txt710 bytespcambra
#50 2321969-50.patch64.8 KBpcambra
#46 2321969-46.patch70.55 KBrpayanm
#46 2321969-interdiff.txt3.9 KBrpayanm
#43 2321969-43.patch69.59 KBrpayanm
#43 2321969-interdiff.txt1.75 KBrpayanm
#1 drupal8-entity_system-file-load-2321969-1.patch58.91 KBTemoor
#2 drupal8-entity_system-file-load-2321969-2.patch58.91 KBTemoor
#5 drupal8-entity_system-file-load-2321969-5.patch58.88 KBTemoor
#5 interdiff-2-5.txt578 bytesTemoor
#10 drupal8-entity_system-file-load-2321969-10.patch58.46 KBTemoor
#10 interdiff-5-10.txt4.93 KBTemoor
#14 drupal8-entity_system-file-load-2321969-14.patch57.95 KBrosinegrean
#16 interdiff-14-16.txt1.01 KBrosinegrean
#16 drupal8-entity_system-file-load-2321969-16.patch58.34 KBrosinegrean
#18 interdiff-16-18.txt65.34 KBrosinegrean
#18 drupal8-entity_system-file-load-2321969-18.patch69.1 KBrosinegrean
#20 interdiff-18-20.txt10.91 KBrosinegrean
#20 drupal8-entity_system-file-load-2321969-20.patch70.9 KBrosinegrean
#23 drupal8-entity_system-file-load-2321969-23.patch67.08 KBrosinegrean
#28 interdiff-23-28.txt2.44 KBrosinegrean
#28 drupal8-entity_system-file-load-2321969-28.patch67.3 KBrosinegrean
#33 2321969-33.patch67.26 KBrpayanm
#36 2321969-36.patch67.29 KBrpayanm
#38 2321969-interdiff.txt1.52 KBrpayanm
#38 2321969-38.patch68.81 KBrpayanm
#42 2321969-interdiff.txt1.73 KBrpayanm
#42 2321969-42.patch131.59 KBrpayanm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Temoor’s picture

Status: Active » Needs review
FileSize
58.91 KB

Not sure should LoadTest remain or be removed.

Temoor’s picture

Issue tags: +#ams2014contest
FileSize
58.91 KB

Previous patch contains tries to get entity.storage instead of entity.manager

Status: Needs review » Needs work

The last submitted patch, 2: drupal8-entity_system-file-load-2321969-2.patch, failed testing.

The last submitted patch, 1: drupal8-entity_system-file-load-2321969-1.patch, failed testing.

Temoor’s picture

Removed getter with empty string.

Temoor’s picture

Status: Needs work » Needs review

Triggering bot

Michael Hodge Jr’s picture

Status: Needs review » Reviewed & tested by the community

I applied the patch in #5, grepped the code, and can confirm everything looks as it should.

dawehner’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/file/file.module
    @@ -1285,12 +1285,7 @@ function file_managed_file_value(&$element, $input, FormStateInterface $form_sta
    -        $fids = array();
    -        foreach ($input['fids'] as $fid) {
    -          if ($file = file_load($fid)) {
    -            $fids[] = $file->id();
    -          }
    -        }
    +        $fids = array_keys(File::loadMultiple($input['fids']));
    
    @@ -1308,12 +1303,7 @@ function file_managed_file_value(&$element, $input, FormStateInterface $form_sta
         if (!empty($default_fids)) {
    -      $fids = array();
    -      foreach ($default_fids as $fid) {
    -        if ($file = file_load($fid)) {
    -          $fids[] = $file->id();
    -        }
    -      }
    +      $fids = array_keys(File::loadMultiple($default_fids));
         }
       }
    

    Just an idea, we could use an entity query to just ensure that all these fids exists. There is no reason to actually load them.

  2. +++ b/core/modules/file/file.module
    @@ -1333,11 +1323,11 @@ function file_managed_file_validate(&$element, FormStateInterface $form_state) {
    +    $files = File::loadMultiple($element['fids']['#value']);
    +    foreach ($element['fids']['#value'] as $fid) {
    ...
    +          $references = \Drupal::service('file.usage')->listUsage($files[$fid]);
    

    Just in case you want, you could extract that service() call above the foreach loop. This just makes it maybe a bit easier to read.

  3. +++ b/core/modules/file/file.module
    @@ -1548,7 +1538,7 @@ function template_preprocess_file_link(&$variables) {
    -  $file_entity = ($file instanceof File) ? $file : file_load($file->fid);
    +  $file_entity = ($file instanceof File) ? $file : File::load($file->fid);
    

    This instanceof should actually check the interface, not the File entity itself.

  4. +++ b/core/modules/file/src/Tests/CopyTest.php
    @@ -39,7 +41,11 @@ function testNormal() {
    -    $this->assertFileUnchanged($result, file_load($result->id(), TRUE));
    +    $fid = $result->id();
    +    $file_storage = $this->container->get('entity.manager')->getStorage('file');
    +    $file_storage->resetCache(array($fid));
    +
    +    $this->assertFileUnchanged($result, File::load($fid));
    
    @@ -66,9 +72,13 @@ function testExistingRename() {
    +    $file_storage = $this->container->get('entity.manager')->getStorage('file');
    +    $file_storage->resetCache(array($source->id()));
    +    $loaded_source = File::load($source->id());
    +    $file_storage->resetCache(array($target->id()));
    +    $loaded_target = File::load($target->id());
    +    $file_storage->resetCache(array($result->id()));
    +    $loaded_result = File::load($result->id());
    
    @@ -106,9 +116,13 @@ function testExistingReplace() {
    +    $file_storage = $this->container->get('entity.manager')->getStorage('file');
    +    $file_storage->resetCache(array($source->id()));
    +    $loaded_source = File::load($source->id());
    +    $file_storage->resetCache(array($target->id()));
    +    $loaded_target = File::load($target->id());
    +    $file_storage->resetCache(array($result->id()));
    +    $loaded_result = File::load($result->id());
    

    Did you tried to actually get rid of the resetting? I could imagine that it won't be needed here but in case it is needed some documentation why it is, would be cool

  5. +++ b/core/modules/file/src/Tests/FileFieldRevisionTest.php
    @@ -47,7 +49,7 @@ function testRevisions() {
    -    $node_file_r2 = file_load($node->{$field_name}->target_id);
    +    $node_file_r2 = File::load($node->{$field_name}->target_id);
    

    I always thought entity API would somehow support that automatically, so something like $node->{$field_name}->getEntity() but i cannot find an example for that. It would be cool if you have a look whether this is possible

Temoor’s picture

getEntity() comes from FieldItemList and returns the entity that field belongs to.
It's possible to get file from $node->{$field_name}->referencedEntities(), that returns array of all entities referenced in this field, but, as long as file is cached in entity storage, is there any advantage comparing to File::load()?
It would be something like this:

$file_array = $node->{$field_name}->referencedEntities();
$node_file_r2 = reset($file_array);

CopyTest runs fine without file cache reset, but as i understand from

Reload the file from the database and check that the changes were actually saved.

it also verifies that file is correctly saved in database, or this test shouldn't worry about this?

Temoor’s picture

Status: Needs work » Needs review
FileSize
58.46 KB
4.93 KB

Attached patch contains fixes 1-4 from #8

Status: Needs review » Needs work

The last submitted patch, 10: drupal8-entity_system-file-load-2321969-10.patch, failed testing.

rosinegrean’s picture

Assigned: Unassigned » rosinegrean
Issue tags: -
rosinegrean’s picture

Status: Needs work » Needs review
FileSize
57.95 KB

Rerolled patch. No new changes.

Status: Needs review » Needs work

The last submitted patch, 14: drupal8-entity_system-file-load-2321969-14.patch, failed testing.

rosinegrean’s picture

Status: Needs work » Needs review
FileSize
1.01 KB
58.34 KB

Fixed the broken tests.

skipyT’s picture

Status: Needs review » Needs work

Nice work, I added some comments also. We should use the entity storage injected everywhere we can do that instead of the static method call.

  1. +++ b/core/modules/editor/src/Form/EditorImageDialog.php
    @@ -208,7 +209,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +      $file = File::load($fid);
    

    We should inject the entity storage into the form class and use the entity storage load method instead of the static call. Here you can find an example how to inject a service into forms: https://www.drupal.org/node/2203931

  2. +++ b/core/modules/file/file.module
    @@ -659,9 +659,10 @@ function file_cron() {
    +    $file_usage = \Drupal::service('file.usage');
    

    same here, we should inject the file.usage service into the form.

  3. +++ b/core/modules/file/file.module
    @@ -1116,7 +1117,7 @@ function file_managed_file_process($element, FormStateInterface $form_state, $fo
    +  $element['#files'] = !empty($fids) ? File::loadMultiple($fids) : FALSE;
    

    here also, use the file storage instead of the static method call

  4. +++ b/core/modules/file/file.module
    @@ -1285,12 +1286,10 @@ function file_managed_file_value(&$element, $input, FormStateInterface $form_sta
    +        $fids = \Drupal::entityQuery('file')
    +          ->condition('fid', $input['fids'], 'IN')
    +          ->exists('fid')
    +          ->execute();
           }
    

    nice improvement!

  5. +++ b/core/modules/file/file.module
    @@ -1333,11 +1330,12 @@ function file_managed_file_validate(&$element, FormStateInterface $form_state) {
    +     $files = File::loadMultiple($element['fids']['#value']);
    +     $file_usage = \Drupal::service('file.usage');
    

    let's use the service here.

  6. +++ b/core/modules/file/file.module
    @@ -1548,7 +1546,7 @@ function template_preprocess_file_link(&$variables) {
    +  $file_entity = ($file instanceof FileInterface) ? $file : File::load($file->fid);
    

    here also

  7. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileFormatterBase.php
    @@ -30,7 +31,7 @@ public function prepareView(array $entities_items) {
    +      $files = File::loadMultiple($fids);
    

    we should inject here the entity storage and use that instead of the static method call

  8. +++ b/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php
    @@ -323,7 +324,7 @@ public static function validateMultipleCount($element, FormStateInterface $form_
    +        $file = File::load($fid);
    

    here also

  9. +++ b/core/modules/file/src/Tests/CopyTest.php
    @@ -39,7 +41,8 @@ function testNormal() {
    +    $this->assertFileUnchanged($result, File::load($fid));
    

    and everywhere where we can use the entity storage we should do it instead of the static method call

  10. +++ b/core/modules/file/src/Tests/FileFieldPathTest.php
    @@ -38,7 +40,9 @@ function testUploadPath() {
    +    $node_file = File::load($node->{$field_name}->target_id);
    

    here you have already the storage, use that instead

rosinegrean’s picture

Status: Needs work » Needs review
FileSize
65.34 KB
69.1 KB

Re-factored to use entity.storage

Status: Needs review » Needs work

The last submitted patch, 18: drupal8-entity_system-file-load-2321969-18.patch, failed testing.

rosinegrean’s picture

Status: Needs work » Needs review
FileSize
10.91 KB
70.9 KB

I'm not sure that in ResponsiveImageFormatter i did the right thing, but the tests for them passed.

Status: Needs review » Needs work

The last submitted patch, 20: drupal8-entity_system-file-load-2321969-20.patch, failed testing.

rosinegrean’s picture

Status: Needs work » Needs review
FileSize
67.08 KB

Rerolled the patch

rpayanm’s picture

Assigned: rosinegrean » Unassigned
Status: Needs review » Reviewed & tested by the community

Look fine for me :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 23: drupal8-entity_system-file-load-2321969-23.patch, failed testing.

skipyT’s picture

  1. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileFormatterBase.php
    @@ -8,11 +8,61 @@
    +   *   The node storage.
    

    We are injecting here the file storage.

  2. +++ b/core/modules/file/src/Tests/FileFieldDisplayTest.php
    --- a/core/modules/file/src/Tests/FileFieldPathTest.php
    +++ b/core/modules/file/src/Tests/FileFieldPathTest.php
    
    +++ b/core/modules/responsive_image/src/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php
    @@ -51,11 +52,13 @@ class ResponsiveImageFormatter extends ImageFormatterBase implements ContainerFa
    +   *   The responsive image mapping storage.
    

    The file storage. would be better here...

rosinegrean’s picture

Status: Needs work » Needs review
FileSize
2.44 KB
67.3 KB

dahousecat’s picture

Patch #28 had a conflict /core/modules/file/file.module around line 634, however once this was resolved it applied cleanly.

Ashok Negi’s picture

Status: Needs review » Needs work

The latest patch does not apply to latest checkout. Changing to Needs Work.

The last submitted patch, 28: drupal8-entity_system-file-load-2321969-28.patch, failed testing.

rpayanm’s picture

Status: Needs work » Needs review
FileSize
67.26 KB

reroll from #28

LinL queued 33: 2321969-33.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 33: 2321969-33.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 36: 2321969-36.patch, failed testing.

rpayanm’s picture

Status: Needs work » Needs review
FileSize
1.52 KB
68.81 KB

Status: Needs review » Needs work

The last submitted patch, 38: 2321969-38.patch, failed testing.

Status: Needs work » Needs review

rpayanm queued 38: 2321969-38.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 38: 2321969-38.patch, failed testing.

rpayanm’s picture

Status: Needs work » Needs review
FileSize
131.59 KB
1.73 KB
rpayanm’s picture

FileSize
1.75 KB
69.59 KB

#42 :ignore:

YesCT’s picture

Issue summary: View changes
Issue tags: -Novice

Seem like this might be ready for a review.
Reviewer should look back at comments mentioned here in the past and make sure the concerns have been addressed.
If a reviewer uses unix grep/ag or other things that helped them do their review, please include those details when posting your review comment.

Taking off novice since this is quite a large patch to manage.
https://www.drupal.org/core-mentoring/novice-tasks

pcambra’s picture

Status: Needs review » Needs work

A couple of small things:

  /**
   * Retrieves a sample file of the specified type.
   */
  function getTestFile($type_name, $size = NULL) {
    // Get a file to upload.
    $file = current($this->drupalGetTestFiles($type_name, $size));

    // Add a filesize property to files as would be read by file_load().
    $file->filesize = filesize($file->uri);

file_load reference in comments should be replaced.

In the method testMultiple() of LoadTest.php:

$this->assertEqual(1, count($by_path_files), 'file_load_multiple() returned an array of the correct size.');

This should be file_storage->loadMultiple() instead

+++ b/core/modules/editor/src/Form/EditorImageDialog.php
@@ -15,6 +15,8 @@
+use Drupal\Core\Entity\EntityStorageInterface;
+use Symfony\Component\DependencyInjection\ContainerInterface;

Shouldn't the use sentences be in alphabetical order?

+++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileFormatterBase.php
@@ -8,11 +8,62 @@
+use Drupal\Core\Entity\EntityStorageInterface;
+use Symfony\Component\DependencyInjection\ContainerInterface;
+use Drupal\Core\Field\FieldDefinitionInterface;
+use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
+use Drupal\file\FileStorageInterface;

Same goes here, there are a couple of more but not sure how important they are.

--- a/core/modules/system/tests/modules/entity_crud_hook_test/entity_crud_hook_test.module
+++ b/core/modules/system/tests/modules/entity_crud_hook_test/entity_crud_hook_test.module

@@ -6,6 +6,7 @@
+use Drupal\file\Entity\File;

Added, but unused

rpayanm’s picture

Status: Needs work » Needs review
FileSize
3.9 KB
70.55 KB
pcambra’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine now.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 46: 2321969-46.patch, failed testing.

dawehner’s picture

Issue tags: +Needs reroll

.

pcambra’s picture

Status: Needs work » Needs review
FileSize
64.8 KB
pcambra’s picture

Issue tags: -Needs reroll

Status: Needs review » Needs work

The last submitted patch, 50: 2321969-50.patch, failed testing.

pcambra’s picture

Status: Needs work » Needs review
FileSize
710 bytes
65 KB
LinL’s picture

LinL’s picture

FileSize
65.26 KB

Another reroll following #2348875: Improving our dump files

prashantgoel’s picture

FileSize
65.11 KB

Rerolled the patched so that it can be applied.

Status: Needs review » Needs work

The last submitted patch, 56: 2321969-rerolled-56.patch, failed testing.

pcambra’s picture

Error reported by testbot is:

Recoverable fatal error: Argument 8 passed to Drupal\image\Plugin\Field\FieldFormatter\ImageFormatter::__construct() must implement interface Drupal\file\FileStorageInterface, instance of Drupal\Core\Session\AccountProxy given, called in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php on line 93 and defined in Drupal\image\Plugin\Field\FieldFormatter\ImageFormatter->__construct() (line 72 of /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php).

+++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
@@ -65,9 +66,11 @@ class ImageFormatter extends ImageFormatterBase implements ContainerFactoryPlugi
-  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings, AccountInterface $current_user, LinkGeneratorInterface $link_generator) {
-    parent::__construct($plugin_id, $plugin_definition, $field_definition, $settings, $label, $view_mode, $third_party_settings);
+  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings, FileStorageInterface $file_storage, AccountInterface $current_user, LinkGeneratorInterface $link_generator) {
+    parent::__construct($plugin_id, $plugin_definition, $field_definition, $settings, $label, $view_mode, $third_party_settings, $file_storage);

Here, some of these parameters must have changed order.

prashantgoel’s picture

FileSize
65.33 KB

As per directions in comment #58.

prashantgoel’s picture

Status: Needs work » Needs review
cilefen’s picture

Grepping for usages in core with #59 applied:

$ egrep -r "(file_load\(|file_load_multiple\()|entity_load\(\'file'|entity_load_multiple\(\'file')" core/
core//CHANGELOG.txt:    * Files are now first class Drupal objects with file_load(), file_save(),
core//modules/file/file.module: *   Whether to reset the internal file_load_multiple() cache.
core//modules/file/file.module: * @see file_load()
core//modules/file/file.module:function file_load_multiple(array $fids = NULL, $reset = FALSE) {
core//modules/file/file.module: *   Whether to reset the internal file_load_multiple() cache.
core//modules/file/file.module: * @see file_load_multiple()
core//modules/file/file.module:function file_load($fid, $reset = FALSE) {
core//modules/file/src/Tests/DeleteTest.php:    // Clear out the call to hook_file_load().
core//modules/file/src/Tests/LoadTest.php: * Tests the file_load() function.
core//modules/file/src/Tests/LoadTest.php:    $this->assertTrue($by_fid_file->file_test['loaded'], 'file_test_file_load() was able to modify the file during load.');
core//modules/file/src/Tests/LoadTest.php:    $this->assertEqual(1, count($by_path_files), 'file_storage->file_load_multiple() returned an array of the correct size.');
core//modules/file/src/Tests/LoadTest.php:    $this->assertTrue($by_path_file->file_test['loaded'], 'file_test_file_load() was able to modify the file during load.');
core//modules/file/src/Tests/LoadTest.php:    $this->assertTrue($by_fid_file->file_test['loaded'], 'file_test_file_load() was able to modify the file during load.');
core//modules/file/src/Tests/LoadTest.php:      $this->assertTrue($by_uuid_file->file_test['loaded'], 'file_test_file_load() was able to modify the file during load.');
core//modules/file/tests/file_test/file_test.module:function file_test_file_load($files) {
core//modules/system/tests/modules/entity_crud_hook_test/entity_crud_hook_test.module:function entity_crud_hook_test_file_load() {
+++ b/core/modules/editor/src/Form/EditorImageDialog.php
@@ -8,13 +8,15 @@
 use Drupal\Component\Utility\Bytes;
-use Drupal\Core\Form\FormBase;
-use Drupal\Core\Form\FormStateInterface;
-use Drupal\filter\Entity\FilterFormat;
 use Drupal\Core\Ajax\AjaxResponse;
+use Drupal\Core\Ajax\CloseModalDialogCommand;
 use Drupal\Core\Ajax\HtmlCommand;
+use Drupal\Core\Entity\EntityStorageInterface;
+use Drupal\Core\Form\FormBase;
+use Drupal\Core\Form\FormStateInterface;
 use Drupal\editor\Ajax\EditorDialogSave;
-use Drupal\Core\Ajax\CloseModalDialogCommand;
+use Drupal\filter\Entity\FilterFormat;

Reordering these makes the patch harder to review.

cilefen’s picture

Status: Needs review » Needs work

Good work: This is almost RTBC.

The comment referring to file_load() in DeleteTest may be wrong.

The class comment on LoadTest is probably wrong.

prashantgoel’s picture

Status: Needs work » Needs review
FileSize
65.32 KB

@Cliefen: I have corrected the patch as per your direction.

Berdir’s picture

Component: entity system » file.module
cilefen’s picture

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

@prashantgoel: Please post interdiffs with the patches.

+++ b/core/modules/file/src/Tests/LoadTest.php
@@ -8,7 +8,7 @@
+ * Tests the load('file') function.

This is better, however, it is still a bit unclear. How about "Tests the file storage load() function."?

subhojit777’s picture

Assigned: Unassigned » subhojit777
subhojit777’s picture

Assigned: subhojit777 » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
62.4 KB

Status: Needs review » Needs work

The last submitted patch, 67: replace_all_instances-2321969-67.patch, failed testing.

subhojit777’s picture

Assigned: Unassigned » subhojit777
subhojit777’s picture

Assigned: subhojit777 » Unassigned
Status: Needs work » Needs review
FileSize
872 bytes
62.41 KB

Status: Needs review » Needs work

The last submitted patch, 70: replace_all_instances-2321969-70.patch, failed testing.

subhojit777’s picture

Assigned: Unassigned » subhojit777
subhojit777’s picture

Assigned: subhojit777 » Unassigned
Status: Needs work » Needs review
FileSize
1.94 KB
63.19 KB
JeroenT’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileFormatterBase.php
    @@ -7,8 +7,11 @@
    +use Drupal\Core\Entity\EntityStorageInterface;
    +use Drupal\Core\Field\FieldDefinitionInterface;
     use Drupal\Core\Field\Plugin\Field\FieldFormatter\EntityReferenceFormatterBase;
     use Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem;
    +use Drupal\file\FileStorageInterface;
    

    Remove unused use statements.

  2. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileFormatterBase.php
    @@ -7,8 +7,11 @@
    +use Drupal\Core\Entity\EntityStorageInterface;
    +use Drupal\Core\Field\FieldDefinitionInterface;
     use Drupal\Core\Field\Plugin\Field\FieldFormatter\EntityReferenceFormatterBase;
     use Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem;
    +use Drupal\file\FileStorageInterface;
    

    Unused use statements.

  3. +++ b/core/modules/file/src/Tests/CopyTest.php
    @@ -39,7 +40,8 @@ function testNormal() {
    +    $fid = $result->id();
    +    $this->assertFileUnchanged($result, $file_storage->load($fid));
    

    Is there a reason we do this on 2 lines and everywhere else on 1 line?

  4. +++ b/core/modules/file/src/Tests/FileFieldTestBase.php
    @@ -40,7 +40,8 @@ function getTestFile($type_name, $size = NULL) {
    +    // Add a filesize property to files as would be read by
    +    // \Drupal\Core\Entity\EntityInterface::load().
    

    Create a todo of this?

  5. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
    @@ -14,6 +14,7 @@
     use Drupal\Core\Session\AccountInterface;
     use Drupal\Core\Url;
     use Drupal\Core\Utility\LinkGeneratorInterface;
    +use Drupal\file\FileStorageInterface;
     use Symfony\Component\DependencyInjection\ContainerInterface;
     use Drupal\Core\Form\FormStateInterface;
     
    @@ -65,9 +66,11 @@ class ImageFormatter extends ImageFormatterBase implements ContainerFactoryPlugi
    
    @@ -65,9 +66,11 @@ class ImageFormatter extends ImageFormatterBase implements ContainerFactoryPlugi
        *   The current user.
        * @param \Drupal\Core\Utility\LinkGeneratorInterface $link_generator
        *   The link generator service.
    +   * @param \Drupal\file\FileStorageInterface $file_storage
    +   *   The file storage.
        */
    -  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings, AccountInterface $current_user, LinkGeneratorInterface $link_generator) {
    -    parent::__construct($plugin_id, $plugin_definition, $field_definition, $settings, $label, $view_mode, $third_party_settings);
    +  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings, AccountInterface $current_user, LinkGeneratorInterface $link_generator, FileStorageInterface $file_storage) {
    +    parent::__construct($plugin_id, $plugin_definition, $field_definition, $settings, $label, $view_mode, $third_party_settings, $file_storage);
         $this->currentUser = $current_user;
         $this->linkGenerator = $link_generator;
       }
    @@ -85,7 +88,8 @@ public static function create(ContainerInterface $container, array $configuratio
    
    @@ -85,7 +88,8 @@ public static function create(ContainerInterface $container, array $configuratio
           $configuration['view_mode'],
           $configuration['third_party_settings'],
           $container->get('current_user'),
    -      $container->get('link_generator')
    +      $container->get('link_generator'),
    +      $container->get('entity.manager')->getStorage('file')
         );
       }
     
    

    it's no longer necessary to pass the file_storage since the FileFormatterBase class now extends the EntityReferenceFormatterBase: #2405469: FileFormatterBase should extend EntityReferenceFormatterBase

  6. +++ b/core/modules/rdf/src/Tests/FileFieldAttributesTest.php
    @@ -68,7 +68,9 @@ protected function setUp() {
    +    $file_storage = $this->container->get('entity.manager')->getStorage('file');
    +    $this->file = $file_storage->load($this->node->{$this->fieldName}->target_id);
    +
     
    

    remove unnecessary newlines.

  7. +++ b/core/modules/responsive_image/src/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php
    @@ -17,6 +17,7 @@
    @@ -53,11 +54,13 @@ class ResponsiveImageFormatter extends ImageFormatterBase implements ContainerFa
    
    @@ -53,11 +54,13 @@ class ResponsiveImageFormatter extends ImageFormatterBase implements ContainerFa
        *   The view mode.
        * @param array $third_party_settings
        *   Any third party settings.
    +   * @param \Drupal\file\FileStorageInterface $file_storage
    +   *   The file storage.
        * @param \Drupal\Core\Entity\EntityStorageInterface $responsive_image_style_storage
        *   The responsive image style storage.
        */
    -  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings, EntityStorageInterface $responsive_image_style_storage) {
    -    parent::__construct($plugin_id, $plugin_definition, $field_definition, $settings, $label, $view_mode, $third_party_settings);
    +  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings, FileStorageInterface $file_storage, EntityStorageInterface $responsive_image_style_storage) {
    +    parent::__construct($plugin_id, $plugin_definition, $field_definition, $settings, $label, $view_mode, $third_party_settings, $file_storage);
     
         $this->responsiveImageStyleStorage = $responsive_image_style_storage;
       }
    @@ -74,6 +77,7 @@ public static function create(ContainerInterface $container, array $configuratio
    
    @@ -74,6 +77,7 @@ public static function create(ContainerInterface $container, array $configuratio
           $configuration['label'],
           $configuration['view_mode'],
           $configuration['third_party_settings'],
    +      $container->get('entity.manager')->getStorage('file'),
           $container->get('entity.manager')->getStorage('responsive_image_style')
         );
       }
    

    it's no longer necessary to pass the file_storage since the FileFormatterBase class now extends the EntityReferenceFormatterBase: #2405469: FileFormatterBase should extend EntityReferenceFormatterBase

Other than that, patch looks ok to me.

subhojit777’s picture

Assigned: Unassigned » subhojit777
subhojit777’s picture

Assigned: subhojit777 » Unassigned
Status: Needs work » Needs review
FileSize
5.78 KB
60.76 KB

Thanks @JeroenT for the review. #74.4 - it is not commented code. It is actually multiline comment. I have updated the patch.

Have not run any tests after making the changes, lets see what testbot says.

Status: Needs review » Needs work

The last submitted patch, 76: replace_all_instances-2321969-76.patch, failed testing.

subhojit777’s picture

Assigned: Unassigned » subhojit777
subhojit777’s picture

We are not using the file_storage class, therefore the tests are failing. I am not exactly sure why this is happening. Since the FileFormatterBase class now extends the EntityReferenceFormatterBase what changes will there be in the code.

JeroenT’s picture

@subhojit777,

+++ b/core/modules/responsive_image/src/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php
@@ -74,6 +75,7 @@ public static function create(ContainerInterface $container, array $configuratio
+      $container->get('entity.manager')->getStorage('file'),

I think the tests are failing because we still pass the fileStorage.

subhojit777’s picture

Assigned: subhojit777 » Unassigned
Status: Needs work » Needs review
FileSize
789 bytes
60.4 KB

@JeroenT Thanks!! I missed that completely.

JeroenT’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/responsive_image/src/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php
    @@ -17,6 +17,7 @@
    +use Drupal\file\FileStorageInterface;
    

    Unused use statement.

  2. +++ b/core/modules/responsive_image/src/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php
    @@ -57,7 +58,7 @@ class ResponsiveImageFormatter extends ImageFormatterBase implements ContainerFa
    +    parent::__construct($plugin_id, $plugin_definition, $field_definition, $settings, $label, $view_mode, $third_party_settings, $responsive_image_style_storage);
    

    I'm sorry, I missed this one. We don't have to pass the responsive_image_style_storage. Same as file_storage in #74.5

After this I'm happy to RTBC this.

subhojit777’s picture

Status: Needs work » Needs review
FileSize
1.77 KB
60.32 KB

Status: Needs review » Needs work

The last submitted patch, 83: replace_all_instances-2321969-83.patch, failed testing.

subhojit777’s picture

Assigned: Unassigned » subhojit777

Sorry to submit the bad patch. I will upload a good patch.

subhojit777’s picture

Issue tags: +SprintWeekend2015
subhojit777’s picture

Assigned: subhojit777 » Unassigned
Status: Needs work » Needs review
FileSize
1.5 KB
60.13 KB
JeroenT’s picture

@subhojit777, i think we can remove the $responsive_image_style_storage out of parent::_construct in responsiveImageFormatter.

Once that is done, i'll mark this as RTBC.

subhojit777’s picture

@JeroenT

  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings, EntityStorageInterface $responsive_image_style_storage) {
    parent::__construct($plugin_id, $plugin_definition, $field_definition, $settings, $label, $view_mode, $third_party_settings);

    $this->responsiveImageStyleStorage = $responsive_image_style_storage;
  }

We are using $this->responsiveImageStyleStorage in many places in ResponsiveImageFormatter. If we remove this then the tests are breaking. Any idea how we can resolve this.

rosinegrean’s picture

@subhojit777

Actually, in the latest patch I can't find any usage for it ...

subhojit777’s picture

@JeroenT Yes it is working after I remove $responsive_image_style_storage from parent construct. Thanks for the review.

@prics Please see ResponsiveImageFormatter.php file, you will not find the usage in patch.

JeroenT’s picture

Status: Needs review » Reviewed & tested by the community

@subhojit777,

Great work! I think this is RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 91: replace_all_instances-2321969-91.patch, failed testing.

JeroenT’s picture

Issue tags: +Needs reroll
rpayanm’s picture

Status: Needs review » Needs work

The last submitted patch, 95: 2321969-95.patch, failed testing.

Status: Needs work » Needs review

rpayanm queued 95: 2321969-95.patch for re-testing.

JeroenT’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
@@ -195,8 +195,8 @@ protected function doLoadMultiple(array $ids = NULL) {
-    // Set default language to current language if not provided.
-    $values += array($this->langcodeKey => $this->languageManager->getCurrentLanguage()->getId());
+    // Set default language to site default if not provided.
+    $values += array($this->langcodeKey => $this->languageManager->getDefaultLanguage()->getId());

+++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
@@ -502,7 +502,7 @@ public function getRouteName() {
-    return (isset($view_route_names[$view_route_key]) ? $view_route_names[$view_route_key] : "view.$view_route_key");
+    return (isset($view_route_names[$view_route_key]) ? $view_route_names[$view_route_key] : "views.$view_route_key");

+++ b/core/modules/views/tests/src/Unit/Plugin/display/PathPluginBaseTest.php
@@ -312,24 +312,6 @@ public function testAlterRoutesWithOptionalParameters() {
-   * Tests the getRouteName method.
-   */
-  public function testGetRouteName() {
-    list($view) = $this->setupViewExecutableAccessPlugin();
-
-    $display = array();
-    $display['display_plugin'] = 'page';
-    $display['id'] = 'page_1';
-    $display['display_options'] = array(
-      'path' => 'test_route',
-    );
-    $this->pathPlugin->initDisplay($view, $display);
-    $route_name = $this->pathPlugin->getRouteName();
-    // Ensure that the expected routename is returned.
-    $this->assertEquals('view.test_id.page_1', $route_name);
-  }
-
-  /**

+++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityStorageTest.php
@@ -139,8 +139,8 @@ protected function setUp() {
-      ->method('getCurrentLanguage')
-      ->willReturn(new Language(array('id' => 'hu')));
+      ->method('getDefaultLanguage')
+      ->will($this->returnValue(new Language(array('langcode' => 'en'))));

@@ -175,7 +175,6 @@ protected function setUp() {
-    $container->set('language_manager', $this->languageManager);

@@ -231,34 +230,6 @@ public function testCreate() {
-   * @covers ::create
-   * @covers ::doCreate
-   */
-  public function testCreateWithCurrentLanguage() {
-    $this->languageManager->expects($this->any())
-      ->method('getLanguage')
-      ->with('hu')
-      ->willReturn(new Language(array('id' => 'hu')));
-
-    $entity = $this->entityStorage->create(array('id' => 'foo'));
-    $this->assertSame('hu', $entity->language()->getId());
-  }
-
-  /**
-   * @covers ::create
-   * @covers ::doCreate
-   */
-  public function testCreateWithExplicitLanguage() {
-    $this->languageManager->expects($this->any())
-      ->method('getLanguage')
-      ->with('en')
-      ->willReturn(new Language(array('id' => 'en')));
-
-    $entity = $this->entityStorage->create(array('id' => 'foo', 'langcode' => 'en'));
-    $this->assertSame('en', $entity->language()->getId());
-  }
-

There is a lot of unrelated change in this patch - looks like a reroll gone wrong. I've not looked for it all... this needs careful redoing are rebase a good patch - I think that's the patch in #87

rosinegrean’s picture

Status: Needs work » Needs review
FileSize
60.85 KB

I rerolled the patch from #87 i think it should be ok now.

rpayanm’s picture

FileSize
1.15 KB
59.7 KB

Missing change from #91

Mile23’s picture

Status: Needs review » Needs work

I don't see the changes mentioned in #99.

I changed file_load() to f_ile_load() (and similar with file_load_multiple()), and ran unit tests. I didn't see any failures or errors.

The real signifier of whether this patch is complete will come in #2352979: Remove file_load_multiple from file/file.module and #2352987: Remove file_load from file/file.module when we find out for sure whether the usages in test were all changed, and those issues can solve those problems when they show up.

One coding standards error:

+++ b/core/modules/file/src/Tests/FileFieldTestBase.php
@@ -45,7 +45,10 @@ function getTestFile($type_name, $size = NULL) {
     $file = current($this->drupalGetTestFiles($type_name, $size));
 
-    // Add a filesize property to files as would be read by file_load().
+    /**
+     * Add a filesize property to files as would be read by
+     * \Drupal\Core\Entity\EntityInterface::load().
+     */
     $file->filesize = filesize($file->uri);

Use // for inline comments. Just wrap them to the next line at 80.

LinL’s picture

Status: Needs work » Needs review
FileSize
59.68 KB
686 bytes
LinL’s picture

FileSize
59.61 KB
560 bytes
+++ b/core/modules/image/src/Tests/ImageFieldDisplayTest.php

@@ -259,8 +261,8 @@ function testImageFieldSettings() {
     // Add alt/title fields to the image and verify that they are displayed.
     $image = array(
       '#theme' => 'image',
-      '#uri' => file_load($node->{$field_name}->target_id)->getFileUri(),
-      '#alt' => $alt,
+      '#uri' => $file_storage->load($node->{$field_name}->target_id)->getFileUri(),
+      '#alt' => $this->randomMachineName(),

The '#alt' here looks like an unrelated change.

LinL’s picture

FileSize
58.99 KB
560 bytes

And a few other unrelated changes in core/modules/migrate_drupal/src/Tests/d6/MigrateFileTest.php (see interdiff)

LinL’s picture

FileSize
1.63 KB

Oops, sorry, that's the old interdiff in #105. Let's try again.

LinL’s picture

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

Patch in #105 no longer applies, tagging for reroll.

rpayanm’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
59.02 KB
Mile23’s picture

Status: Needs review » Needs work

All the unrelated stuff from #99 on is gone, there's lots of happy dependency injection, and it passes tests. I couldn't find instances of file_load(), file_load_multiple(), entity_load('file') or entity_load_multiple('file').

RTBC except for this one thing:

+++ b/core/modules/file/src/Tests/LoadTest.php
@@ -68,16 +70,16 @@ function testMultiple() {
     $by_path_files = entity_load_multiple_by_properties('file', array('uri' => $file->getFileUri()));
     $this->assertFileHookCalled('load');
-    $this->assertEqual(1, count($by_path_files), 'file_load_multiple() returned an array of the correct size.');
+    $this->assertEqual(1, count($by_path_files), 'file_storage->file_load_multiple() returned an array of the correct size.');

The assertion message isn't accurate. entity_load_multiple_by_properties() returned the array. Also, there's no such thing as file_storage->file_load_multiple(). :-) Just change the message and we're good.

Sumi’s picture

Status: Needs work » Needs review
FileSize
59.02 KB
973 bytes
JeroenT’s picture

Issue tags: +drupaldevdays

Patch no longer applied. Created re-roll.

JeroenT’s picture

And here is the patch!

Status: Needs review » Needs work

The last submitted patch, 112: replace_all_instances-2321969-111.patch, failed testing.

JeroenT’s picture

Status: Needs work » Needs review
FileSize
59.94 KB

.

Status: Needs review » Needs work

The last submitted patch, 114: replace_all_instances-2321969-114.patch, failed testing.

JeroenT’s picture

Status: Needs work » Needs review
FileSize
59.85 KB
572 bytes
Mile23’s picture

Issue tags: +Needs reroll

Needs a reroll.

Pretty sure the changes from #109 happened, and so it looks pretty good.

Mile23’s picture

Status: Needs review » Needs work
JeroenT’s picture

Status: Needs work » Needs review
FileSize
57.31 KB

Created reroll.

Mile23’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

Thanks folks.

The patch in #119 does in fact remove calls to: file_load(), file_load_multiple(), entity_load('file') and entity_load_multiple('file').

file_load() and file_load_multiple() are marked as deprecated for D9.

entity_load() and entity_load_multiple() are not deprecated.

The work here is already done, however, and internal consistency is good: #2471679: [Meta] Make core internally consistent

The beta evaluation is in the meta: #2225347: [META] Replace calls to ENTITY_TYPE_load() and ENTITY_TYPE_load_multiple() with static method calls

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Thanks everyone for your ongoing work on this! As a reminder, while these functions are deprecated for 9.0 and not 8.0.0, @alexpott and I agreed to grant them an exception and continue the cleanup during the beta. Details in #2225347: [META] Replace calls to ENTITY_TYPE_load() and ENTITY_TYPE_load_multiple() with static method calls.

Onto the patch. A few general points:

  • In general, this patch is a lot bigger than it needs to be. There are some out of scope changes that should be removed. Especially in issues of this nature, it's very important to restrict the changes to the minimum necessary.
  • I'd also prefer that we use File::load() in tests; after all, the summary of this issue is "replace... with static method calls." (Failing that though, we should at least get the service only once per test class and put it in a protected property. I'd prefer File::load() in tests though for simplicity. See also #2087751: [policy, no patch] Stop using $this->container in web tests which @Mile23 found for me again.)
  • There are a few places that replace mentions of the old function in comments; these should always refer to the default implementation \Drupal\blahnamespace\File::load().
  • Finally, we need to follow Drupal API documentation standards.

Some specific feedback:

  1. +++ b/core/modules/editor/src/Form/EditorImageDialog.php
    @@ -8,13 +8,15 @@
     use Drupal\Component\Utility\Bytes;
    -use Drupal\Core\Form\FormBase;
    -use Drupal\Core\Form\FormStateInterface;
    -use Drupal\filter\Entity\FilterFormat;
     use Drupal\Core\Ajax\AjaxResponse;
    +use Drupal\Core\Ajax\CloseModalDialogCommand;
     use Drupal\Core\Ajax\HtmlCommand;
    +use Drupal\Core\Entity\EntityStorageInterface;
    +use Drupal\Core\Form\FormBase;
    +use Drupal\Core\Form\FormStateInterface;
     use Drupal\editor\Ajax\EditorDialogSave;
    -use Drupal\Core\Ajax\CloseModalDialogCommand;
    +use Drupal\filter\Entity\FilterFormat;
    +use Symfony\Component\DependencyInjection\ContainerInterface;
    

    Alphabetizing use statements is out of scope for this issue (and not even actually a standard AFAIK). Let's revert that so there isn't stuff adding noise to review; the patch is big enough.

  2. +++ b/core/modules/editor/src/Form/EditorImageDialog.php
    @@ -22,6 +24,29 @@
    +   * @var EntityStorageInterface $file_storage
    +   */
    +  protected $fileStorage;
    

    This parameter needs a one-line summary explaining what it is.

  3. +++ b/core/modules/editor/src/Form/EditorImageDialog.php
    @@ -22,6 +24,29 @@
    +   * The constructor.
    

    I can see that it's the constructor by looking at its name. :) This should be something like "Constructs a form object for image dialogues" or such.

  4. +++ b/core/modules/editor/src/Form/EditorImageDialog.php
    @@ -22,6 +24,29 @@
    +   * @param EntityStorageInterface $file_storage
    

    This parameter documentation needs a description.

  5. +++ b/core/modules/file/file.module
    @@ -626,9 +627,10 @@ function file_cron() {
    +    $file_usage = \Drupal::service('file.usage');
    ...
    -      $references = \Drupal::service('file.usage')->listUsage($file);
    +      $references = $file_usage->listUsage($file);
    

    This change is not in scope and should be reverted.

  6. +++ b/core/modules/file/file.module
    @@ -1211,7 +1213,8 @@ function template_preprocess_file_link(&$variables) {
    -  $file_entity = ($file instanceof File) ? $file : file_load($file->fid);
    +  $file_storage = \Drupal::entityManager()->getStorage('file');
    +  $file_entity = ($file instanceof FileInterface) ? $file : $file_storage->load($file->fid);
    

    This also includes an out of scope change (checking instanceof the interface rather than the class). Also, why are we not just using File::load()? It's procedural code.

  7. +++ b/core/modules/file/src/Tests/CopyTest.php
    @@ -17,6 +17,7 @@ class CopyTest extends FileManagedUnitTestBase {
    +    $file_storage = $this->container->get('entity.manager')->getStorage('file');
    
    @@ -64,11 +65,12 @@ function testExistingRename() {
    +    $file_storage = $this->container->get('entity.manager')->getStorage('file');
    
    @@ -104,11 +106,12 @@ function testExistingReplace() {
    +    $file_storage = $this->container->get('entity.manager')->getStorage('file');
    

    Since these are in the same test class, can we add this to setup instead and put it in a protected class member? That, or just do File::load() (as above). There are many more instances where I would provide this feedback in the patch.

  8. +++ b/core/modules/file/src/Tests/DeleteTest.php
    @@ -40,9 +42,9 @@ function testInUse() {
    -    // Clear out the call to hook_file_load().
    +    // Reset the call to load('file').
    

    I don't think this change is correct. hook_file_load() still exists; it's a specific case of hook_ENTITY_TYPE_load(). See: https://www.drupal.org/node/2294409

  9. +++ b/core/modules/file/src/Tests/LoadTest.php
    @@ -8,7 +8,7 @@
    - * Tests the file_load() function.
    + * Tests the load('file') function.
    

    It's not the load('file') function. It certainly won't work if you try to load the string 'file'. Better would be something like: "Tests \Drupal\blah...\load()."

  10. +++ b/core/modules/file/src/Tests/LoadTest.php
    @@ -68,16 +70,16 @@ function testMultiple() {
    -    $this->assertEqual(1, count($by_fid_files), 'file_load_multiple() returned an array of the correct size.');
    +    $this->assertEqual(1, count($by_fid_files), 'file_storage->loadMultiple() returned an array of the correct size.');
    

    file-storage->loadMultiple() isn't a thing. Here (and elsewhere in documentation) we should use \Drupal\...\File::load().

  11. +++ b/core/modules/file/src/Tests/SaveUploadTest.php
    @@ -100,13 +101,13 @@ function testNormal() {
    +    // Load both files using $file_storage->loadMultiple().
    

    \Drupal\...blah\File::load() in docs (sorry lost the hunk above where I already said this).

  12. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
    @@ -74,6 +75,8 @@ class ImageFormatter extends ImageFormatterBase implements ContainerFactoryPlugi
    +   * @param \Drupal\Core\Entity\EntityStorageInterface $image_style_storage
    +   *   The image style entity storage.
    

    This change, while correct, is out of scope.

  13. +++ b/core/modules/image/src/Tests/ImageFieldDefaultImagesTest.php
    @@ -253,7 +253,8 @@ public function testDefaultImages() {
    -    $file = File::load($default_images['field_new']->id());
    +    $file_storage = $this->container->get('entity.manager')->getStorage('file');
    +    $file = $file_storage->load($default_images['field_new']->id());
    
    @@ -297,7 +298,7 @@ public function testDefaultImages() {
    -    $file = File::load($default_images['field_new']->id());
    +    $file = $file_storage->load($default_images['field_new']->id());
    

    I actually think this is out of scope; it's already not using the deprecated method, so this doesn't help the goals of the patch.

Let's try to make a new patch that minimizes the changed lines. Thanks!

xjm’s picture

If #121 seems a bit overwhelming, also take a look at #2322195: Replace all instances of user_load(), user_load_multiple(), entity_load('user') and entity_load_multiple('user') with static method calls, which is more in line with what these patches should do. :)

rosinegrean’s picture

Thanks @xjm, i will work on your comments

rosinegrean’s picture

1, 2, 3, 4 done
5. reverted
6, 7 reverted to File::load() in all tests
8, 9, 10, 11 fixed
12. removed, maybe create a task for it ?
13. reverted

One small question: when doing File::load() and the cache needs to be reseted, is there another way to do it, besides
\Drupal::entityManager()->getStorage('file')->resetCache(array($fid)); ?

Because if we need to get the storage to do the reset cache, then maybe it's better to leave it like it is.

rosinegrean’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 124: replace_all_instances-2321969-124.patch, failed testing.

JeroenT’s picture

Issue tags: +Needs reroll
JeroenT’s picture

Mile23’s picture

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

Here's a reroll.

Mile23’s picture

FileSize
46.85 KB
1.17 KB

I went through to double-check if #121 was addressed completely. Looks good on that front.

I only found a couple of coding standards errors, fixed here.

daffie’s picture

The patch looks good to me. Just two questions:

+++ b/core/modules/file/src/Tests/FileFieldWidgetTest.php
@@ -220,6 +221,7 @@ function testMultiValuedWidget() {
+    $file_storage = $this->container->get('entity.manager')->getStorage('file');

Why is this line added. The variable $file_storage is not used anywhere.

+++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
@@ -15,6 +15,7 @@
+use Drupal\file\FileStorageInterface;

@@ -96,7 +97,8 @@ public static function create(ContainerInterface $container, array $configuratio
-      $container->get('entity.manager')->getStorage('image_style')
+      $container->get('entity.manager')->getStorage('image_style'),
+      $container->get('entity.manager')->getStorage('file')

Why are these lines added. Can you explain.

JeroenT’s picture

Status: Needs work » Needs review
FileSize
1.49 KB
45.3 KB

@daffie, you're right. Those changes were not necessary. Created a new patch.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. To RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed faada60 and pushed to 8.0.x. Thanks!

  • alexpott committed faada60 on 8.0.x
    Issue #2321969 by rpayanm, subhojit777, prics, LinL, JeroenT, Temoor,...

Status: Fixed » Closed (fixed)

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