This summary is adapted from #2818227: Undeprecate entity_get_(form_)display() or offer a real replacement for accuracy.
Problem/Motivation
#2474151: Mark procedural wrappers in entity.inc as deprecated deprecated entity_get_display() and entity_get_form_display(), but it did not offer real replacements, except to (eugh) copy the whole function body.
The point of those helper functions is that you don't know if an entity display exists, and they're designed to be created when used the first time. So it is very common to not know if one exists or not. The functionality is useful and should be preserved.
Proposed resolution
A) Keep the functions deprecated; there is no good reason for them to remain as procedural functions except as wrappers around a service.
B) Offer an actual replacement. According to the BC policy, it's OK to add new methods in minor releases.
Remaining tasks
Review and commit the patch.
User interface changes
None.
API changes
Two new methods added to EntityDisplayRepositoryInterface and all implementations -- currently EntityDisplayRepository and EntityManager. That means we need to add two new but already deprecated methods to EntityManager, but that is required because it implements that interface.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#83 | interdiff-2367933-80-83.txt | 980 bytes | phenaproxima |
#83 | 2367933-83.patch | 247.57 KB | phenaproxima |
#81 | interdiff-2367933-78-80.txt | 1.11 KB | phenaproxima |
#81 | 2367933-80.patch | 246.47 KB | phenaproxima |
#78 | interdiff-2367933-73-78.txt | 16.12 KB | phenaproxima |
Comments
Comment #1
amateescu CreditAttribution: amateescu commentedCan't decide between
getEntity(View|Form)Display()
andget(View|Form)Display()
.. I chose the former for the initial patch.Comment #3
amateescu CreditAttribution: amateescu commented#notmyday
Comment #5
jhedstromThese have since been
@deprecated
for methods on the entity type manager.Comment #6
BerdirThey are, but that was IMHO a bad call, because there is no replacement for it, the point of those functions is that they create a display on demand if necessary and I don't want to duplicate that everywhere where we need it.
Comment #8
phenaproximaArise, Lazarus. I agree with @Berdir -- these functions are hella useful. They deserve to be deprecated, but not removed -- they should be part of a service. That is what I have done; both are now methods of EntityDisplayRepositoryInterface.
Comment #10
phenaproximaBleh. Forgot to update EntityManager.
Comment #11
BerdirI think I'm getting old. Closed #2818227: Undeprecate entity_get_(form_)display() or offer a real replacement as a duplicate. that also says why adding those methods is OK in a minor release.
should this use static?
the example needs to be updated to use the non-deprecated approach.
should mention what we default to.
We don't need add deprecated methods for something that was never here in the first place :)
Should we do a few example conversions? Looks like we only have a handful of use cases outside of functions/tests, so possibly we could convert those? (some field_ui forms)
Comment #12
phenaproximaAll fixed except for:
#1. We cannot use static to refer to interface constants because static is a compile-time construct.
#4. EntityManager must implement the methods, because it implements EntityDisplayRepositoryInterface.
Comment #14
phenaproximaD'oh. Let's try that again.
Comment #15
BerdirI think this is ready, but this possibly needs a change record to clarify that those functions are still deprecated but now have an actual replacement.
We probably also want to take over parts or all of the issue summary of #2818227: Undeprecate entity_get_(form_)display() or offer a real replacement, including the reasoning why adding those methods to an existing interface is OK, per our BC rules.
Comment #16
phenaproximaChange record written: https://www.drupal.org/node/2835616
Also updated #2818227: Undeprecate entity_get_(form_)display() or offer a real replacement.
Comment #17
BerdirCopied from IRC, in case you don't see it there:
I think you misunderstood "We probably also want to take over parts or all of the issue summary of ..." :)
I'm saying this has a very old and wrong issue summary. we should update it and the one from that other issue should be a good starting point. Or just that, now that you updated it there ;)
Comment #18
phenaproximaComment #19
phenaproximaComment #20
phenaproximaComment #21
BerdirThanks.
Comment #22
alexpottI think we should use this issue to add the missing explicit test coverage of the new methods on the Entity Display repository.
Ugh what a shame that this has to be done. But no avoiding that.
Can we search for and if it does not exist create followups to remove the usages of entity_get_display() and entity_get_form_display() - there are plenty.
Just a random thought but over in #2834268: DX of \Drupal::config() is extremely confusing: doesn't complain when requesting non-existing config people are having issues with the auto-create if it does not exist approach. Will we have an issue with that here?
Comment #23
phenaproximaTo paraphrase Dr. Zoidberg, what's the point of keeping the usages around after this patch? It's scope creep, but IMHO we should just remove all usages now. Like taking off a band-aid.
Here's a WIP patch that removes many usages from core, mostly in tests.
Comment #24
phenaproximaComment #25
phenaproximaNow that was a dirty job. Someone should call Mike Rowe.
Anyhoo, this patch removes all usages of entity_get_display() and entity_get_form_display() -- most of which were in tests. Let's see how many failures we get!
Decided not to post an interdiff because there's nothing really special going on here; it's just changing function calls to service calls. If all goes well, will add explicit tests of both methods.
Comment #27
phenaproximaI forgot a semicolon.
Comment #29
phenaproximaFixed borked tests and added explicit test coverage of both new methods. Should be good to go!
Comment #30
BerdirHm. I'm not sure if converting all tests in here is a good idea, makes for a prety huge patch.
See also #2066993: Use \Drupal consistently in tests for discussion on $this->container vs \Drupal in tests.
Comment #31
phenaproximaIt was either going to be a huge patch in a follow-up issue, or a huge patch in this one. I figure we should just get it done now and be through with it.
Comment #32
phenaproximaAfter discussion with @Berdir on IRC, here are two things that should help this get going:
1) A review-only patch containing the relevant changes, without any of the updated code.
2) Calls to $this->container were replaced with \Drupal.
Comment #33
slashrsm CreditAttribution: slashrsm at MD Systems GmbH commentedA nit:
I believe that the second line needs to be indented.
Comment #35
mpdonadioNeeds a re-roll, likely b/c #2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase, but looks like some other conflicts, too.
Comment #36
jofitz CreditAttribution: jofitz at ComputerMinds commentedI have a feeling I might regret this, but I'll give it a shot...
Comment #37
jofitz CreditAttribution: jofitz at ComputerMinds commentedI was right - I did regret doing that! But here is the fruit of my labour.
Comment #39
chr.fritschJust a re-roll on 8.5.x
Comment #40
chr.fritschI removed all the remaining occurrences of entity_get_display and entity_get_form_display
Comment #42
phenaproximaOkay...
Comment #43
phenaproximaRe-rolled!
Comment #44
phenaproximaRemoved more calls to entity_get_display() and entity_get_form_display().
Comment #47
andypostThis needs rework
1) no reason to add methods to deprecated EM
2) displays just needs custom storage classes to hide logic with entity creation of default display
Maybe better to override load() method on the entities or use post_load hook?
this methods are not defined initially, no reason to add deprecated methods to deprecated interface
Comment #48
phenaproximaEither way is valid. We need a framework manager to break this stalemate.
So the question I pose to them is this: should we...
$display_storage->load($id, $create_if_not_exists = FALSE)
?Tagging for framework manager review before we proceed with this.
Comment #49
andypostI prefer this approach but not sure optional argument allowed by interface
Comment #50
phenaproxima@andypost: My only hesitation with that approach is that it's not clear how we should handle loadMultiple(). Should the create-if-not-exists flag work only for load(), or for loadMultiple() as well? Because it might be pretty tricky to make it work for loadMultiple() too.
Comment #52
andypostFaced with that again working on domain_entity (menu) module - some distributions are missing form displays for menu link entities...
Btw looking through usage of "EGFD" it is used exactly for such ugly cases when entities have no default display
Probably for 9.x we need to prevent having "missing but default" display usage at all
As workaround entity repository looks a best place cos it already has ETM injected & implements similar "hacky methods" which works abroad all entities
Comment #54
volegerComment #55
Berdir> Tagging for framework manager review before we proceed with this.
I believe that this is not a framework-level decision but one for an entity subsystem manager. I happen to be one and I prefer to have them on the EntityDisplayRepository.
Here is a rebase, conflicted on a bunch of tests but most things still applied thanks to applying the patch to a commit from 2017 and then letting git rebase do its magic. A few conflicts I just dropped, they'll need to be redone in their new locations.
Originally, I was against doing this in a single 200kb patch, but with our new rules, this is now the way it has to be.
Next steps:
* Add @trigger_error() statements to the deprecated functions
* Make changed constructor arguments optional, again with @trigger_error()
* Update all remaining instances.
Comment #58
andypostThe most usage of new API in install & tests (which mix "full" and "default") - also it makes in interface that default display always exists (imo no reason to separate default view mode variable name and have 2 constants in interface)
Also I find it useful, for search module at east, to get display from repository/factory in controllable status (true or false)
Maybe better to factor interface to ($entity_type_id, $bundle, $mode='default', $status=TRUE) - view or form already used in method name
Comment #59
larowlanI think they should be on EntityDisplayRepository too, and if that means we need to create a new interface to avoid adding them to EntityManager, then let's do that
Comment #60
BerdirRe #58: Yeah, agree, the constants can be merged. But I don't think the status is necessary.
Re #59: We actually added quite a new methods to EntityManager, do we really need to make an exception here? I find that new interfaces are tricky as all implementations need to implement that anyway or the service woudln't work. Recent examples: #2984782: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data and #2942907: Entity system does not provide an API for retrieving an entity variant that is safe for editing.
Just moving this a bit along, added @trigger_error() and updated the remaining usages. Messed up the interdiff by working on it during rebase, but it's just more of the same.
Comment #61
alexpottI'm upgrading this to a major task since this will help people trying to fix deprecated code since as we know atm the procedural methods have no direct counterparts and that makes getting your code ready for Drupal 9 harder than it should be.
Comment #62
mikelutzComment #63
mikelutzTypo fix to move this forward. Looking to fix the deprecation in contrib, and would prefer to fix it with the final api if possible.
Comment #64
mikelutzTest fixes, removed an unneeded 'default' argument.
Comment #66
mikelutzFix for coding standards
Comment #67
BerdirAdded legacy tests, also reformatted things a bit, I didn't really like that "single line" implementation in EntityViewDisplay.
I think this is ready for final reviews.
Comment #68
amateescu CreditAttribution: amateescu at Pfizer, Inc. commentedThis looks really good to me. I mostly found nitpicks but setting to NW for point #5 :)
as of drupal:8.0.0
->in drupal:8.0.0
?Missing space before
$storage
:)Exceeds 80 chars.
Since we're changing these lines anyway, we can use
@covers ...
.I'm guessing these tests are added because of a bad merge, so they should be removed from the patch.
Comment #69
volegerAddressed #68
Comment #71
volegerComment #73
johnwebdev CreditAttribution: johnwebdev commentedFixes tests :)
Comment #74
amateescu CreditAttribution: amateescu at Pfizer, Inc. commentedI think we're good to go now!
Comment #75
larowlanWe have a mismatch in versions here.
This was actually deprecated in #2474151: Mark procedural wrappers in entity.inc as deprecated which was 8.0.0, so we should probably retain that?
do we normally document the type of constants using @var? Looked at a few samples in core, e.g. CommentItemInterface and FieldStorageDefinitionInterface and didn't see it.
we can inject this
And here too
and here
Comment #76
Berdir1. Missed the inconsistent one. IMHO, the version defines when it is safe to use the documented replacement. That is 8.8.0, the earlier deprecation was was a a lie as it didn't have an actual replacement.
Will fix that and the injections when I find some time, but someone else can try that too if they want.
Comment #77
alexpottI agree with the rationale for changing the deprecation to 8.8.0 as that’s when you can use the replacement. The other question is should we file an issue to undeprecate these methods in 8.7.x as well - which I would be in favour of as a followup.
Comment #78
phenaproximaAddressing #75:
Comment #79
Berdirindentation wrong on the second line.
It looks like comment.services.yml hasn't been updated for the new argument?
Comment #81
phenaproximaAh, well spotted! Both fixed. I bet this will also cause way fewer tests to fail. :)
Comment #83
phenaproximaFixing the final broken test.
Comment #84
BerdirI think the review has been addressed. Not sure about the constant myself, I've never added @var to them before, but it indeed looks there is no standard and if there are recent examples in core that have a type then I'll defer that decision to whoever is going to commit this. Can easily be removed before commit too.
The only examples I found is https://www.drupal.org/docs/develop/standards/api-documentation-and-comm... in the context of an event and https://www.drupal.org/docs/develop/standards/api-documentation-samples#..., but saw many empty and out of date examples on that page, including "Member constant" a bit up.
Comment #85
larowlanIssue credits
Comment #86
larowlanfixed on commit
Committed da94d7f and pushed to 8.8.x. Thanks!
Published change record