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.
Beta phase evaluation
Issue category | Task, as it makes core more consistent, helps contrib/custom module authors |
---|---|
Issue priority | Major, because it allows us to get rid of quite some code |
Disruption | Unless someone would have subclassed those specific classes, there would be no API change. Existing configurations have just to be adapted for schema purposes, the existing views will continue to work, see
|
Problem/Motivation
There are a lot of code out there to provide some kind of links/information for all entities in views.
- Add a link to the entity view page.
- Add a link to the entity edit form.
- Add a link to the entity delete form.
- Show the human readable version of the bundle of the current result
- ...
https://www.drupal.org/node/2281185 has a couple of those added.
Proposed resolution
Implement all kind of generic entity handlers and drop existing code in as many places as possible.
Remaining tasks
User interface changes
API changes
See beta evaluation.
Comment | File | Size | Author |
---|---|---|---|
#112 | views-entity_links-2322949-112.patch | 86.34 KB | plach |
Comments
Comment #1
dawehnerHere is s start.
Comment #3
dawehnerWorking on it.
Comment #4
dawehner.
Comment #6
jhodgdonCan we get an actual issue summary? ... The patch looks interesting, but it would be good to know what problem it is supposed to be solving. :)
Comment #7
dawehnerThat is maybe a bit better ... sorry.
Comment #8
jhodgdonAh thanks. That is kind of what I thought by reading the patch; good to confirm that this is the intention.
A few notes on the current patch:
a) The error here is real:
Fatal error: Uncaught exception 'LogicException' with message 'Missing phpDoc on Drupal\views\Tests\Handler\FieldEntityTest.' in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/src/TestDiscovery.php:394
This file in the patch has a pretty-much empty test in it with no docs header; test runner cannot handle this.
b) Generally I think this patch looks good! Making the link stuff standardized across entities seems like an excellent idea.
c) It also looks like you're doing something about consolidating bundle stuff? Such as in NodeViewsData:
That may be missing from the issue summary? Or should that be a separate fix? Doesn't seem related to these links....
d)
The docs header is wrong, I think... what is this actually?
e)
The first line here... what is this supposed to mean? I don't understand it.
f) Looks like a few tests still need to be filled in?
Comment #9
dawehnerThank you for providing so much feedback in various issues in a reasonable timeframe, this is really valuable, as you know.
Well, the issue started to merge in some code from the "entity_views" module, but yeah I am not sure whether this approach now is valid.
entity_views is a nice playground for better integrations we want to have in core as well. One random example which also exists now is proper support for UUIDs everywhere you can reference to an entity in views.
Yeah copy and paste error, sorry :) Will improve things on my next run unless someone beets me
TRUE
Comment #10
jhodgdonIt seems like maybe on this issue, we should limit it to making a unified method for entity view/edit/delete/translate links (is that the full list?). And if there are other nice things in entity_views, we can get them on other issues. That should keep things manageable?
I guess my other question after looking at this patch is whether we could maybe get rid of some of the specific handler plugins, like aggregator/src/Plugin/views/field/TitleLink.php and ... are the specific types of links needed, like DeleteLink and EditLink, or could they be taken care of with one master EntityLink class that has some ... what's it called, not configuration but when you specify the options for the handler in the hook_views_data()? Might make the patch even smaller.
Comment #11
dawehnerOKay sure, let's remove the bundle field for now, see #2363811: Add a generic entity bundle field
Whoever continues to work on this patch, the bundle code should be removed, if possible.
Comment #12
jibranIs this same as #2363811: Add a generic entity bundle field?
Comment #13
chx CreditAttribution: chx commentedit's clear that contrib (ex-core in this case) needs this, see #2409887: Views integration
Comment #14
BerdirFirst attempt at reroll, tried to remove the bundle and view stuff, removed the empty test classes.
Comment #16
BerdirConfig schema fixes.
Comment #18
BerdirMore schema fixes.
Comment #20
jhodgdon+100000000! This is great (plus or minus a few test failures).
Comment #21
dawehnerComment #22
yched CreditAttribution: yched commentedLooks awesome, even though I'm probably not the right person for a thorough review.
Just noticed :
So does renderLink() need to check the $entity or not ?
Comment #23
BerdirSome test fixes, tricky stuff.
Comment #24
BerdirComment #26
jibranRelated #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency
Comment #27
jhodgdonI related #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality to this issue as well. The summary on that issue lists all of the specific field handler plugins in Core that we'd like to standardize and get rid of if possible, several of which are links. For instance, in the Node section there are:
node_link
node_link_edit
node_link_delete
node_revision_link
node_revision_link_revert
node_revision_link_delete
User, Comment, and Taxonomy have additional link fields, and some of the others have things similar to the generic "node", which prints the node title and allows you to make it a link (which #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency is taking out anyway in favor of using the generic "field" handler).
There is also content_translation_link, which is added to most entities.
So... It looks like the latest patch here doesn't actually get rid of most of the handler classes (yet), and I don't think it covers all of the cases above, but it looks like a good start. One idea would be to reduce this patch further by having it create the infrastructure for doing this, and actually only remove the specific handlers for one entity (like Node), and then do the others in follow-up patches, rather than trying to do all of them at once? Since we have that other meta issue now, we won't lose track of the fact that not everything was converted to use the new system.
Comment #28
jhodgdon#2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality is now a critical meta-issue.
On that issue, we need to convert a lot of entity link views pseudo-fields to use some kind of entity-aware rendering in Views rather than one-off plugins. This issue would presumably solve the problem, and due to the number of entities and pseudo-fields that are depending on this, I think this issue is at least Major if not Critical.
Comment #29
dawehnerThis is not true ... those fields are not driven by entity fields but instead they are just ordinary views field plugins ... so you really don't have to care about them here ..
but no question, we really want have them to be generic so people have no problems with exposing them with their custom entity types.
Comment #30
damiankloip CreditAttribution: damiankloip commentedStart with a reroll. There have been quite some changes in core since the last patch.
Comment #32
damiankloip CreditAttribution: damiankloip commentedconflict fail, resolving conflict in YAML files is HORRIBLE :)
Yeah, there are certain things that we will still need specialist field implementations for, like most of these, but the entity system is such that we should be able to solve most cases generically.
Comment #34
dawehnerNice!
Comment #35
jhodgdonSo... I took a look at this patch.
A few comments/concerns:
a) The aggregator stuff:
- The aggregator title field plugin is going away on #2456701: Replace aggregator_title_link Views formatter with Field API formatter, which is about 99% RTBC and should be completed today, so all the Aggregator code in the patch is not necessary.
b) But anyway, it's kind of a different beast from the others, so I think it wouldn't have been good to include it here... I think the point of this issue is to replace the generic "Make a link to X" fields, the ones that allow you to define the link text in Views (with some plain text or presumably a views-token-replacement-thing). So for instance, the "make a generic link to the node" field just makes a link to node/12345 and you define the link text. Every other plain-text field (not only Title but any now) also allows you to turn it into a link to the entity, but we have these special fields for making plain-old links to view, edit, etc.
So to me, fields like Title (or in this case Aggregator Item Title), are not the same, because instead of putting in some plain-text for the link text, they are instead getting the link text from a specific field, and then allowing you to make it into a link, right? To me, that's different, and I think this patch should just be dealing with those plain-text-make-a-link pseudo-fields, not the this-is-a-field-turn-it-into-a-link... besides which all text fields already have this ability now in Views.
Hope that made sense...
c) I don't think we actually want to turn on the links for all entities in all cases that the entity defines in its canonical link list. So I think maybe we just need a generic method on EntityViewsData that will create such a link field, and then let each specific entity ViewsData class call it to get those methods if it makes sense. ?? Not sure about this, but right now, for instance, Node, Comment, and User have these links, and Taxonomy has some, but File doesn't and Aggregator doesn't... But then again maybe it's good to turn them on generically? OK I think I'm undecided on this, but probably agree with turning it on generically and standardizing all entities. ;)
d) Is it going to be possible to do something about things that aren't canonical entity-defined links? For instance the Node views data defines links for viewing/reverting revisions. These are just defined as generic routes in node.routing.yml, and they're not in the links section of the entity annotation on the Node class. Can anything be done for them?
Comment #36
kgoel CreditAttribution: kgoel commentedWorking on this at Drupal Dev Days...
Comment #37
jhodgdonSitting with kgoel... here are some questions / comments [including the open questions from #35]:
1. Do we want the call to EntityViewsData::addEntityLinks() to be done by default for all entities in EntityViewsData::getViewsData(), or should we (at least initially) just make the method exist, and let Node, Comment, User, and Taxonomy call it in their own Entity ViewsData classes, to reproduce what is in Core now?
2. Here is a list of the existing entity link pseudo-views-fields and plugins. Can we replace all of them in this patch?
- plugin node_link (pseudo-field: view_node)
- node_link_edit (pseudo-field: edit_node)
- node_link_delete (pseudo-field: delete_node)
- node_path (pseudo-field: path)
- node_revision_link (pseudo-field: link_to_revision on node_revision)
- node_revision_link_revert (pseudo-field: revert_revision on node_revision)
- node_revision_link_delete (pseudo-field: delete_revision on node_revision)
- user_link_edit (pseudo-field: UserFieldData - edit_node, which despite the name is actually a link to edit the user)
- user_link_cancel (pseudo-field: UserFieldData - cancel_node, which despite the name is actually a link to cancel the user)
- comment_link: Used in CommentViewsData for pseudo-field view_comment
- comment_link_edit: Used in CommentViewsData for pseudo-field edit_comment
- comment_link_delete: Used in CommentViewsData for pseudo-field delete_comment
- comment_link_approve: Used in CommentViewsData for pseudo-field approve_comment
- comment_link_reply: Used in CommentViewsData for pseudo-field replyto_comment
- term_edit_link - field edit_term
- content_translation_link (Used in pseudo-fields called translation_link in: NodeViewsData, UserViewsData, BlockContentViewsData, CommentViewsData, TermViewsData)
Note: Other entities do not currently have these link fields: File, AggregatorItem, AggregatorFeed, BlockContent
3. Since all text/string fields on Entities (such as Title) now have the ability to link to the entity (canonical link), do you agree that converting title and similar fields is out of scope for this issue? I think we should limit this issue to the plain "link" fields that let you define the link text, and exclude fields like "title with ability to make link" from this one.
4. Note: In the list in item 2 here, some of the link fields are not actually part of the entity-defined link list. For example, on Node, the revision view, delete, and revert links are not part of the Node link annotation, which looks like this:
The revision links are just regular routes, defined in node.routing.yml, like this:
Can we handle them in this issue, or is that out of scope?
Comment #38
jhodgdonProposed answers to the questions in #37:
1. Go ahead and do this for all entities. Will add some links, but that is OK.
2. See item 1 and 4.
3. Limit to the fields listed in 2 or a subset; others out of scope.
4. Limit to fields defined in entity annotation, not routing.yml files like revision revert/delete for node.
Comment #39
jhodgdonGiven these answers, the patch kgoel is about to post will contain:
- A reroll of the previous patch
- Leaving out aggregator stuff [out of scope text fields and covered elsewhere anyway]
- Leaving out the changes to the Drupal\node\Plugin\views\field\Node class [out of scope text fields]
- Added content translation to the mix. Used existing content translation plugin, but edited to subclass the new EntityLink class instead of what it did before ==> no new schema or views changes needed since ID is same and so are options (phew!)
Previous patch didn't exactly apply; interdiff difficult; please excuse the mess. ;)
Comment #40
kgoel CreditAttribution: kgoel commentedNeed to add couple lines for content_translation in EntityViewsDataTest.php
Comment #41
jibran@kgoel interdiff please. :)
Let's add assert for translation link.
Comment #42
kgoel CreditAttribution: kgoel commented@jibran - Please see jhodgdon's comment...
Please see my comment- https://www.drupal.org/node/2322949#comment-9838445
Comment #44
jibranThose line were not there before when I was last here. j/k :P
Don't mind me carry on :)
Comment #45
dawehnerExactly. For the Delete link for example, we want to fallback to t( 'Delete') though, for example.
That would be super super cool!
This statement was true a while ago, but with #2455153: Switch revision log views fields to use 'field' formatter we can check for 'entity.$entity_type.revision', I think.
I would vote for that, given that we head towards that in much more areas now already. Its IMHO just really good DX
MH, maybe we could add them as link templates onto the node entity type.
Yeah, this is kinda a hidden easteregg of shame.
It would be certainly helpful to have that provided for all entity types as well.
Yeah, I absolutely agree.
Well, I handled the renaming as part of #2455153: Switch revision log views fields to use 'field' formatter and noone complained about it :)
Comment #46
kgoel CreditAttribution: kgoel commentedComment #47
jhodgdonThis patch is looking like it's pretty close, assuming the bot agrees. One change crept in by mistake:
This shouldn't have been removed in the patch.
Comment #48
kgoel CreditAttribution: kgoel commentedComment #49
jhodgdonSeveral of the link plugins that are mentioned in comment #37 item 2 are not actually removed or replaced in this patch... so I think there is a bit more work to do.
Comment #52
kgoel CreditAttribution: kgoel commentedComment #53
jhodgdonCloser!
Looks like this still needs some work though:
(and following lines) - I think we need to keep this one. This is some kind of weird link thing that shows links to the node the comment is on or something.
We'll need this: this class (and the reply plugin below) are replaced with new code, but still exist as special plugins.
See comment above.
Still need these, see above.
We're not doing anything to this plugin any more, so just leave this as it was.
Need to also get rid of the section for the now-deleted views.field.user_link too.
This is garbled. It looks like in the reroll something happened? There's a mix of stuff from the language field and the entity link field here....
This is also messed up...
ok this schema file just needs to be redone.
Hm... something missing here?
Comment #55
kgoel CreditAttribution: kgoel commentedI am not sure about #9, need to look into that more.
Comment #56
jhodgdonOK, I think this is all good now except for the @fixme comment and maybe some test fails.
Comment #57
kgoel CreditAttribution: kgoel at Forum One commentedComment #59
kgoel CreditAttribution: kgoel at Forum One commentedComment #61
fgmSome class names have changed (Link became EntityLink). Rerolled.
Comment #63
fgmShould pass better: type-hinting renderLink on EntityInterface on other classes exteding EntityLink.
Comment #65
kgoel CreditAttribution: kgoel at Forum One commentedComment #67
fgmExtra hinting on NodeNewComment.
Also renamed $data to $entity since it implements EntityInterface: this is better for DX.
Comment #68
fgmGaaah, missed interdiff again.
Comment #70
kgoel CreditAttribution: kgoel at Forum One commentedComment #71
dawehnerYes, using 'translation_' . $entity_type_id makes things more consistent, but at the same time, we should think about existing configurations out there. For them, we should better try to not require them to update their views, ... again, so I'd recommend to use 'translation_link' itself. Feel free to disagree
I'm really suprised that its works ... given that nothing seems to ensure that the 'status' column is there. What about pulling it from the entity instead?
If we kinda subclass that entity_link field handler, we don't have to specify the mapping again. So just skip it for entity_link_delete and entity_link_edit
Quick comment: We don't need the 'translatable' here anymore ... That bit just doesn't exist anymore in Drupal 8.
You could probably do it by this single line:
$this->options['alter']['language'] = $this->getEntity($values)->language();
We should use
$this->options['alter']['url'] = $entity->urlInfo('delete-form')
instead, and similar to the edit form.I like that we have additional test coverage in
EntityViewsDataTest
Comment #72
kgoel CreditAttribution: kgoel at Forum One commentedComment #74
kgoel CreditAttribution: kgoel at Forum One commentedComment #76
kgoel CreditAttribution: kgoel at Forum One commentedComment #77
dawehnerThank you, I think its a sane idea to not break existing views if possible. Updated the issue summary
Comment #78
kgoel CreditAttribution: kgoel at Forum One commentedManually tested and I do see view, edit and delete links on entities page.
Comment #79
kgoel CreditAttribution: kgoel at Forum One commentedComment #80
dawehnerWe already have some test coverage for those links, maybe a generic would be nice, so create a view of entity_test entities and check whether those links appear as expected.
I really like this patch so far!
Comment #81
plachThis is blocking #2450897: Cache Field views row output, so promoting to critical, I'll try to complete the work here, since Daniel believes this is almost ready.
Here is a quick review:
Is this correct? Does not look so.
This should be added by the Content Translation module.
Looks like these methods are redundant...
Comment #82
plachComment #83
plachMore accurate title
Comment #84
plachThis merges the work I did in #2450897: Cache Field views row output, basically it provides automatic route access checking.
#81.2 still to be addressed. Let's see how many things I broke.
Comment #86
plachThis should fix the failing tests.
Comment #87
plachThis implements #81.2, working on some additional test coverage now.
Comment #89
plachSome fixes/clean-ups...
Comment #90
plachThis adds a couple of small fixes and the missing test coverage.
Edit: I don't know why but the patch appears only as attachment in the issue summary.
Comment #91
plachAttaching again, just in case
Comment #92
dawehnerThe patch looks really awesome, so much saved code.
Here is a question, why do we override both renderLink and getUrlInfo even the query could be theoretically added to the $url object in getUrlInfo() as well.
Here is a question, why is the default text of $this->options['text'] not directly $this->getDefaultLabel()? The defineOptions() method could use it.
I think we should be able to remove the init() method as well.
Seems to be an accident ...
newline needed ...
... Why do we not inject it, if we already inject the access manager?
As written before, let's use
$options['text'] = ['default' => $this->getDefaultLabel()]
Why do you not use +=? I mean if we replace the entire array we can also remove it with unset().
Contains ...
Nice test coverage!
Comment #93
Fabianx CreditAttribution: Fabianx for Acquia commentedExcept what Daniel found, this would be RTBC for me.
Comment #94
plachThis should address #92. I was performing some manual testing and I noticed revision links are broken in HEAD, I implemented a quick fix, but deferred providing a proper fix and test coverage to #1863898: Add test coverage for Views revision link handlers.
1: That's the only class where doing what you suggest makes sense, for consistency with the others I'd keep it as-is.
6: There's a @todo (see the PHP doc) to add the current user
PluginBase
, so I'd keep it as-is for now.Comment #95
dawehnerThank you! It looks great for me now!
Comment #98
plachGreen, back to RTBC
Comment #99
alexpottSo are we going to leave the post comments check in
CommentController::getReplyForm()
? And if so should we be testing it?Not used.
Comment #100
dawehnerAdded the change record ...
Comment #101
plachThis reverts the changes to the comment reply access control to avoid regressions, as I don't completely understand the current logic and I suspect there might be something I'm missing. Moreover those changes are a bit out of scope here and can be easily addressed as a follow-up.
Comment #102
dawehner@larowlan I guess you understand more so you could file a follow up?
Comment #103
andypostInterdiff is not in last patch - NW
The reason behind reply form to check access within controller is to Give user ability to login and back to reply form, because visitors could came to the replay form via email notifications and getting 403 does not tell exactly what's up...
it's not reverted
Comment #104
plach@larowlan found these two related issues:
#56942: Comment cids should belong to the associated node in comment/reply
#50835: Reply to non existent comments
I think at least the check introduced in the first one was lost in translation. We should probably open a follow-up to verify the new code is still addressing the original use cases and improve it to just use access callback where needed.
Comment #105
plachSorry, forgot to commit. This time for reals.
Comment #106
plachhttps://www.drupal.org/node/2483117
Comment #107
Wim LeersThis looks very close!
The description makes sense, but doesn't explain where that is then happening.
Could use an
@see
.Docs mismatch.
More docs mismatches.
Why not inject, this, like
AccessManager
?Beecause the todo will move it to
PluginBase
, and so injecting it here would cause more work elsewhere?Comment #108
plachYep, and it wouldn't just be more work, it'd also be an API break :)
Comment #109
plachAnd this...
Comment #110
Wim LeersThanks!
Checked that #103 is also addressed. Back to RTBC per #102.
Comment #111
catchDon't really understand the comment. When do we 'pull up the user account'? Was the comment just moved without updating it?
Struggled to find much else though, this really cleans a lot up.
Comment #112
plachThat comment was originally in the
::renderLink()
method, but you are right that now it makes no sense, it's only confusing, removed it. I also removed the::access()
method as its parent implementation is more correct.Comment #113
Fabianx CreditAttribution: Fabianx for Acquia commentedRTBC, looks great!
Comment #114
catchCommitted/pushed to 8.0.x, thanks!
Comment #116
plachYay, thanks!
Published the CR
Comment #117
plachRetroactively tagging, as this was blocking #2450897: Cache Field views row output.
Comment #118
plachComment #121
quietone CreditAttribution: quietone at PreviousNext commentedClosed #2035587: Move Views Translation link definitions from specific entity modules to Content translation module as a duplicate adding credit