On #2900409: [Meta] Improve UI of Reference field settings form, Berdir noticed that the code for that patch was very similar to the views_entity_field_label() function.
Rather than maintaining this code in two places, it would be better if it was moved to be a method on a class accessible as a service. Berdir suggested maybe putting on the EntityFieldManager class.
Move to EntityFieldManager
Make views_entity_field_label() call that method.
Deprecate views_entity_field_label() -- meaning adding @deprecated tag to the doc block, adding trigger_error to the code, and replacing calls to that function in core.
Follow up:
Once that is done, we can also use the new function on #2900409: [Meta] Improve UI of Reference field settings form (which will be taken care of on that issue, not here).
None.
function views_entity_field_label() will be deprecated in favor of \Drupal::service('entity_field.manager')->getFieldLabels($entity_type_id, $field_name);
None.
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
compare changes, plain diff MR !10309
Comments
Comment #2
jhodgdonNote: As a bonus, that function is calling deprecated code (using the deprecated EntityManager) so that will go away too. :) I looked for the issue to remove those deprecations but I didn't locate it...
Comment #3
jhodgdonMy mistake on #2 -- the deprecated code was fixed in 8.8.x already.
In any case, here is a patch that hopefully creates this method on EntityFieldManager, and makes the views function call it. I haven't done the actual deprecation yet, so let's see if the function works first -- hopefully the original function had some kind of test that depended on it... The new method body is a direct copy/paste from the views function, except that I've replaced calls to Drupal:: with using the class itself and a service it already had injected.
I'll also go over to #2900409: [Meta] Improve UI of Reference field settings form and try to modify that patch to use this method.
Comment #4
jhodgdonActually, I went back to #2900409: [Meta] Improve UI of Reference field settings form... That code is doing more than what this function does, so I don't actually think it's useful to call this function... so maybe this issue is not so necessary?
The problem is that for the reference field sort UI (that other issue), we need to go through the field definitions in more detail, because we need to know:
- the most commonly-used label (this function does that)
- whether there was more than one label or not (this function does that)
- whether the field is considered to be Computed (this function does not do that)
- what the columns are in the storage definition (this function does not do that)
So we could call this function to get the label, but since we already need to go through the field information in loops to gather the other information, I don't think it makes a lot of sense to do that.
I'll add a note on the other issue. Anyway, it may still make sense to refactor this function... ??? not sure.
Comment #5
andypostComment #13
smustgrave commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
If I'm understanding the deprecation was figured out in D9 so this should be updated to do the deprecation warning.
Will need a test to show the message.
Comment #16
nicxvan commentedComment #18
nicxvan commentedComment #19
nicxvan commentedComment #20
nicxvan commentedAddressing 13 we don't need tests showing deprecation messages per the deprecation policy.
Comment #21
nicxvan commentedComment #22
nicxvan commentedComment #23
nicxvan commentedComment #24
nicxvan commentedComment #25
berdirThis looks good to me, the only question is about BC, I think EntityFieldManagerInterface is a 1:1 interface and we can add methods there, but it could still break of someone for example decorates that service.
Implementing the interface finds finds only one contrib, and that's a false positive, that has its own Interface, it also doesn't extend core.
https://git.drupalcode.org/search?group_id=2&scope=blobs&search=%22imple...
extends finds a few, two test stubs and one real subclass, those _should_ be fine:
https://git.drupalcode.org/search?group_id=2&scope=blobs&search=%22exten...
Who knows what kind of shenanigans people did with custom code :shrug:
Was about to RTBC, but the change record should have a before/after example even if it's trivial. It possibly should also more clearly say that this method has been added to EntityFieldManagerInterface.
Comment #26
nicxvan commentedI updated the CR thanks!
Comment #27
berdirLooks good to me now.
Comment #28
nicxvan commentedComment #29
quietone commentedThis needs a rebase and I left some comments in the MR.
@nicxvan, why did you tag this as 11.2.0 release priority?
Comment #30
berdir> @nicxvan, why did you tag this as 11.2.0 release priority?
Because this is a blocker to deprecate hook_hook_info() usage, as it relies on those extra include files being loaded.
Comment #31
nicxvan commentedGreat thanks! I applied most suggestions, I'll rebase and address the rest of your comments later today or early tomorrow.
@berdir is right this is one of two blockers to deprecate hook hook info which is the real target.
Comment #32
nicxvan commentedOk I addressed your comments and rebased, I fixed one more typo as well.
I'll keep an eye on tests and move status after.
Comment #33
nicxvan commentedComment #34
berdiradded more more comment that I missed before.
Comment #35
nicxvan commentedRebased and addressed feedback
Comment #36
berdirThanks.
Comment #37
dwwAdded some suggestions for very minor / pedantic nits. Otherwise, seems reasonable to me.
Comment #38
nicxvan commentedTests are green after applying nits.
Comment #39
dwwThanks, looks better.
Issue title and summary look clear and accurate.
Saving credit for quietone and myself for MR reviews.
Don’t see anything else to improve. RTBC++
Comment #40
catchThis looks good but needs a rebase.
Comment #41
nicxvan commentedWorking on this.
Comment #42
nicxvan commentedRebased thanks!
Comment #43
dwwphpstan is failing in the pipeline.
Comment #44
dwwRelated, but the rebase seems to have clobbered views.views.inc entirely. I thought we still needed a deprecated views_entity_field_label() in there.
TL;DR: Probably wise not to self-RTBC after a rebase, especially with merge conflicts, so that peer review can help spot trouble. 😅
Comment #45
nicxvan commentedYeah I'm working on baseline, the deprecated function was moved to views.module
The whole point of this was so we could delete the views.views.inc file, but only the last of the two issues to get in could actually delete it.
Comment #46
nicxvan commentedComment #47
dwwSorry, I missed something in previous reviews (or this is a new bug). 1 suggestion to apply, otherwise seems RTBC to me.
Comment #48
dwwp.s. I did some minor edits and formatting cleanups on the CR: https://www.drupal.org/node/3489411/revisions/view/13800107/13851975
Comment #49
nicxvan commentedComment #50
dwwI see nothing left to improve. RTBC.
Thanks!
-Derek
Comment #52
catchCommitted/pushed to 11.x, thanks!