Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
The entity_view</code and <code>entity_view_multiple
functions have been deprecated for some time now. They should not be used in core as they are deprecated.
Proposed resolution
Remove the final usages from core and trigger a deprecation warning when the functions are called by implementing modules.
Remaining tasks
review
commit
User interface changes
none
API changes
none
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#31 | 2974253-31.drupal.Remove-remaining-usages-of-entityviewmultiple-from-core.patch | 35.42 KB | mikelutz |
#23 | interdiff.txt | 10.88 KB | andypost |
Comments
Comment #2
legolasboComment #4
legolasboThe test failed due to the following error, which seems unrelated to me:
Comment #6
legolasboBack to needs review now the testbot issues have been resolved.
Comment #8
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedLooks good to me.
Comment #9
catchIs there already an issue open to properly deprecate node_view_multiple()?
Comment #10
legolasboI don't think there is @catch, and the same seems to be true for comment_view_multiple(), user_view_multiple() and taxonomy_term_view_multiple(). Those issues should be created, but that shouldn't block this patch from landing should it?
Comment #11
larowlanWe need a test for the deprecation error being triggered
Comment #12
volegerComment #13
andypostAdded CR https://www.drupal.org/node/3033656 which explains deprecation and provides examples
There was no CR for already deprecated methods
Meantime I think we can deprecate node, term and user same time with #2974258: Remove remaining usages of entity_view
Patch fixes message & deprecate remains so I think we can close second issue
Comment #14
andypostIssue needs better title as it have commutative patch, also CR needs review
Comment #15
Berdir+1 to explicitly deprecate all calling functions here as well, we have to touch them anyway. Size of the patch seems managable.
should be Druapl 8.0.0 in all those deprecation messages. I'm also still not sure if it should be drupal:8.0.0 now or "Drupal 8.0.0", the format that was proposed in the issue where that is discussed I think proposes the first, but that is not really used anywhere yet.
seeems a bit pointless to change the old function, but I guess that's because we otherwise get two deprecation messages. doesn't really matter though.
maybe this already has the entity type manager injected? not sure if we should add it here if not.
we know it is contact_message, so not necessary to make it pseudo-generic with that $entity->getEntityTypeId()?
this also knows it is a node..
and this probably too, since it is a test.
this will conflict with [#/2923701] I guess.
Not sure which version we should use as being deprecated here. IMHO the main information of that is which version is safe to rely on. This sounds like the replacement is only available in 8.7, which is not true, but not sure if we have any explicit guidelines on that.
here too, row plugins might have the entity type manager already?
didn't review this up close yet, but the load and delete deprections so far put functions into module specific tests, we could possibly extend those (e.g. they do create load and sometimes delete, so we could put a view in the middle)
Comment #16
andypostFix for #15
1) let's use new format from #3024461: Adopt consistent deprecation format for core and contrib deprecation messages
2) I did convert it because otherwise deprecation will be thrown twice
3) sadly no, probably needs follow-up because it using renderer which already in base class
4-6) fixed
7) nice spot, not sure which one will in first
8) as we deprecate here and message thrown - I guess 8.7
9) same as 3 - just keeping scope
10) tests are split into existing classes, added
\Drupal\KernelTests\Core\Entity\EntityLegacyTestTrait
to not duplicate assertions also added\Drupal\Tests\comment\Kernel\CommentLegacyTest
Comment #17
Gábor Hojtsy@andypost: please don't use the new format until it is added to coding standards, and the deprecation policy is updated. Please stick to the current format.
Comment #18
andypostReverted messages
Comment #19
larowlanPatch at #18 is not consistent with the interdiff - still has the old format - possible that its the same patch as #16 - wrong file?
Comment #20
andypostRe-roll after #2923701: Mechanism to disable preprocessing of node base fields so they can be configured via the field UI & yep, valid patch(
Comment #21
andypostAnother re-roll
Comment #22
BerdirI'm quite sure this is not working at all as we have been lazy-rendering for a long time, so the array key in render() is never going to be set.
Not really our problem here, but still..
in other cases we put multiple methods in the same, you can have multiple expected deprecation messages. don't need the separate method to create the comment then.
this seems pretty bogus, it's a one line assertion that we can also do easily in each case we test it.
also unsure about this one, why not just do assertCount(4, $build) and then the trait isn't needed.
Comment #23
andypostremoved trait and joined deprecations
#22.1 looks needs follow-up but not clear what to write in summary
#22.2
createComment()
method could be inlined but IMO better to have it to minimize patchComment #25
andypostqueued again
Comment #26
andypostComment #28
andypostComment #30
BerdirNeeds a reroll.
Comment #31
mikelutzRe-roll of #23 with no changes.
Comment #32
mikelutzI also opened #3051491: Deprecate user_view and user_view_multiple because, why not! I assume that will need a re-roll after this gets in.
Comment #33
BerdirI'm still not sure if we should deprecate and update, in other places, we explicitly didn't touch deprecated code. The downside is that calling comment_view() would trigger two deprecation messages.
Not a big deal and it's done now already, so will leave the final decision on this to whoever is going to commit this (or not)
Beside that, I think this looks good now. All entity view functions are deprecated, have deprecation tests and all usages are updated.
Comment #34
larowlani'm ok with this, its dead code, its going to go away anyway
Comment #35
larowlanCrediting @Gábor
Comment #36
larowlanCommitted f950d08 and pushed to 8.8.x. Thanks!
Comment #38
jibranPublished the CR.