Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
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
?
Comment | File | Size | Author |
---|---|---|---|
#133 | replace_all_instances-2321969-133.patch | 45.3 KB | JeroenT |
#133 | interdiff.txt | 1.49 KB | JeroenT |
Comments
Comment #1
Temoor CreditAttribution: Temoor commentedNot sure should LoadTest remain or be removed.
Comment #2
Temoor CreditAttribution: Temoor commentedPrevious patch contains tries to get entity.storage instead of entity.manager
Comment #5
Temoor CreditAttribution: Temoor commentedRemoved getter with empty string.
Comment #6
Temoor CreditAttribution: Temoor commentedTriggering bot
Comment #7
Michael Hodge Jr CreditAttribution: Michael Hodge Jr commentedI applied the patch in #5, grepped the code, and can confirm everything looks as it should.
Comment #8
dawehnerJust an idea, we could use an entity query to just ensure that all these fids exists. There is no reason to actually load them.
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.
This instanceof should actually check the interface, not the File entity itself.
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
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
Comment #9
Temoor CreditAttribution: Temoor commentedgetEntity() 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:
CopyTest runs fine without file cache reset, but as i understand from
it also verifies that file is correctly saved in database, or this test shouldn't worry about this?
Comment #10
Temoor CreditAttribution: Temoor commentedAttached patch contains fixes 1-4 from #8
Comment #13
rosinegrean CreditAttribution: rosinegrean commentedComment #14
rosinegrean CreditAttribution: rosinegrean commentedRerolled patch. No new changes.
Comment #16
rosinegrean CreditAttribution: rosinegrean commentedFixed the broken tests.
Comment #17
skipyT CreditAttribution: skipyT commentedNice work, I added some comments also. We should use the entity storage injected everywhere we can do that instead of the static method call.
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
same here, we should inject the file.usage service into the form.
here also, use the file storage instead of the static method call
nice improvement!
let's use the service here.
here also
we should inject here the entity storage and use that instead of the static method call
here also
and everywhere where we can use the entity storage we should do it instead of the static method call
here you have already the storage, use that instead
Comment #18
rosinegrean CreditAttribution: rosinegrean commentedRe-factored to use entity.storage
Comment #20
rosinegrean CreditAttribution: rosinegrean commentedI'm not sure that in ResponsiveImageFormatter i did the right thing, but the tests for them passed.
Comment #23
rosinegrean CreditAttribution: rosinegrean commentedRerolled the patch
Comment #24
rpayanmLook fine for me :)
Comment #27
skipyT CreditAttribution: skipyT commentedWe are injecting here the file storage.
The file storage. would be better here...
Comment #28
rosinegrean CreditAttribution: rosinegrean commentedComment #30
dahousecat CreditAttribution: dahousecat commentedPatch #28 had a conflict /core/modules/file/file.module around line 634, however once this was resolved it applied cleanly.
Comment #31
Ashok Negi CreditAttribution: Ashok Negi commentedThe latest patch does not apply to latest checkout. Changing to Needs Work.
Comment #33
rpayanmreroll from #28
Comment #36
rpayanmComment #38
rpayanmComment #42
rpayanmComment #43
rpayanm#42 :ignore:
Comment #44
YesCT CreditAttribution: YesCT commentedSeem 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
Comment #45
pcambraA couple of small things:
file_load reference in comments should be replaced.
In the method testMultiple() of LoadTest.php:
This should be
file_storage->loadMultiple()
insteadShouldn't the use sentences be in alphabetical order?
Same goes here, there are a couple of more but not sure how important they are.
Added, but unused
Comment #46
rpayanmComment #47
pcambraLooks fine now.
Comment #49
dawehner.
Comment #50
pcambraReroll as this removed a bunch of file_load calls #2357801: File field default values are not deployable -- use UUIDs for the default file
Comment #51
pcambraComment #53
pcambraComment #54
LinL CreditAttribution: LinL commentedReroll following #2394157: Update the EntityFile destination to handle temporary files and #2403873: FileFormatterBase does not retain unsaved entities (files)
Comment #55
LinL CreditAttribution: LinL commentedAnother reroll following #2348875: Improving our dump files
Comment #56
prashantgoel CreditAttribution: prashantgoel commentedRerolled the patched so that it can be applied.
Comment #58
pcambraError 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).
Here, some of these parameters must have changed order.
Comment #59
prashantgoel CreditAttribution: prashantgoel commentedAs per directions in comment #58.
Comment #60
prashantgoel CreditAttribution: prashantgoel commentedComment #61
cilefen CreditAttribution: cilefen commentedGrepping for usages in core with #59 applied:
Reordering these makes the patch harder to review.
Comment #62
cilefen CreditAttribution: cilefen commentedGood work: This is almost RTBC.
The comment referring to file_load() in DeleteTest may be wrong.
The class comment on LoadTest is probably wrong.
Comment #63
prashantgoel CreditAttribution: prashantgoel commented@Cliefen: I have corrected the patch as per your direction.
Comment #64
BerdirComment #65
cilefen CreditAttribution: cilefen commented@prashantgoel: Please post interdiffs with the patches.
This is better, however, it is still a bit unclear. How about "Tests the file storage load() function."?
Comment #66
subhojit777Comment #67
subhojit777Comment #69
subhojit777Comment #70
subhojit777Comment #72
subhojit777Comment #73
subhojit777Comment #74
JeroenTRemove unused use statements.
Unused use statements.
Is there a reason we do this on 2 lines and everywhere else on 1 line?
Create a todo of this?
it's no longer necessary to pass the file_storage since the FileFormatterBase class now extends the EntityReferenceFormatterBase: #2405469: FileFormatterBase should extend EntityReferenceFormatterBase
remove unnecessary newlines.
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.
Comment #75
subhojit777Comment #76
subhojit777Thanks @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.
Comment #78
subhojit777Comment #79
subhojit777We 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.
Comment #80
JeroenT@subhojit777,
I think the tests are failing because we still pass the fileStorage.
Comment #81
subhojit777@JeroenT Thanks!! I missed that completely.
Comment #82
JeroenTUnused use statement.
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.
Comment #83
subhojit777Comment #85
subhojit777Sorry to submit the bad patch. I will upload a good patch.
Comment #86
subhojit777Comment #87
subhojit777Comment #88
JeroenT@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.
Comment #89
subhojit777@JeroenT
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.Comment #90
rosinegrean CreditAttribution: rosinegrean commented@subhojit777
Actually, in the latest patch I can't find any usage for it ...
Comment #91
subhojit777@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.
Comment #92
JeroenT@subhojit777,
Great work! I think this is RTBC.
Comment #94
JeroenTComment #95
rpayanmComment #98
JeroenTComment #99
alexpottThere 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
Comment #100
rosinegrean CreditAttribution: rosinegrean commentedI rerolled the patch from #87 i think it should be ok now.
Comment #101
rpayanmMissing change from #91
Comment #102
Mile23I 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:
Use // for inline comments. Just wrap them to the next line at 80.
Comment #103
LinL CreditAttribution: LinL commentedComment #104
LinL CreditAttribution: LinL commentedThe '#alt' here looks like an unrelated change.
Comment #105
LinL CreditAttribution: LinL commentedAnd a few other unrelated changes in core/modules/migrate_drupal/src/Tests/d6/MigrateFileTest.php (see interdiff)
Comment #106
LinL CreditAttribution: LinL commentedOops, sorry, that's the old interdiff in #105. Let's try again.
Comment #107
LinL CreditAttribution: LinL commentedPatch in #105 no longer applies, tagging for reroll.
Comment #108
rpayanmComment #109
Mile23All 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')
orentity_load_multiple('file')
.RTBC except for this one thing:
The assertion message isn't accurate.
entity_load_multiple_by_properties()
returned the array. Also, there's no such thing asfile_storage->file_load_multiple()
. :-) Just change the message and we're good.Comment #110
Sumi CreditAttribution: Sumi commentedComment #111
JeroenTPatch no longer applied. Created re-roll.
Comment #112
JeroenTAnd here is the patch!
Comment #114
JeroenT.
Comment #116
JeroenTComment #117
Mile23Needs a reroll.
Pretty sure the changes from #109 happened, and so it looks pretty good.
Comment #118
Mile23Comment #119
JeroenTCreated reroll.
Comment #120
Mile23Thanks 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
Comment #121
xjmThanks 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:
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 preferFile::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.)\Drupal\blahnamespace\File::load()
.Some specific feedback:
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.
This parameter needs a one-line summary explaining what it is.
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.
This parameter documentation needs a description.
This change is not in scope and should be reverted.
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.
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.
I don't think this change is correct.
hook_file_load()
still exists; it's a specific case ofhook_ENTITY_TYPE_load()
. See: https://www.drupal.org/node/2294409It'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()."
file-storage->loadMultiple() isn't a thing. Here (and elsewhere in documentation) we should use
\Drupal\...\File::load()
.\Drupal\...blah\File::load() in docs (sorry lost the hunk above where I already said this).
This change, while correct, is out of scope.
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!
Comment #122
xjmIf #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. :)
Comment #123
rosinegrean CreditAttribution: rosinegrean commentedThanks @xjm, i will work on your comments
Comment #124
rosinegrean CreditAttribution: rosinegrean commented1, 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.
Comment #125
rosinegrean CreditAttribution: rosinegrean commentedComment #128
JeroenTComment #129
JeroenTBTW: This is the last sub-issue of #2225347: [META] Replace calls to ENTITY_TYPE_load() and ENTITY_TYPE_load_multiple() with static method calls
Comment #130
Mile23Here's a reroll.
Comment #131
Mile23I 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.
Comment #132
daffie CreditAttribution: daffie commentedThe patch looks good to me. Just two questions:
Why is this line added. The variable $file_storage is not used anywhere.
Why are these lines added. Can you explain.
Comment #133
JeroenT@daffie, you're right. Those changes were not necessary. Created a new patch.
Comment #134
daffie CreditAttribution: daffie commentedLooks good to me. To RTBC.
Comment #135
alexpottCommitted faada60 and pushed to 8.0.x. Thanks!