Issue:

Default field--node--title.html.twig template adds a span, which appears to be for quick edit functionality.

If that's the case, I prefer we don't add that by default in core unless the user has permissions to edit that bundle. I have a quick patch for the approach that I think will work, will be adding it shortly.

If there are other reasons for that span I'd like to figure out a way to only add it if it's going to get used/is needed.

CommentFileSizeAuthor
#1 remove-unneeded-nodes-title-markup-if-user-cant-edit-that-node-bundle-2516412-1.patch647 byteswesruv
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

wesruv’s picture

wesruv CreditAttribution: wesruv at Lullabot commented
FileSize
remove-unneeded-nodes-title-markup-if-user-cant-edit-that-node-bundle-2516412-1.patch647 bytes

Here's a patch using the user.hasPermission method in the twig template.

To Test:

  1. Login as a user that can edit a node.
  2. Inspect node title, notice the span wrapping the text
  3. Look at node as anon (or role that doesn't have permission to edit the node)
  4. Inspect node title, span shouldn't be there
wesruv’s picture

wesruv CreditAttribution: wesruv at Lullabot commented
Status: Needs work » Needs review
wesruv’s picture

wesruv CreditAttribution: wesruv at Lullabot commented
Issue summary: View changes
Related issues: +#2483319: [META] Remove unnecessary markup from core templates, a.k.a. divitis
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented

I think this is problematic because any other modules can add to those attributes (via MODULENAME_preprocess_field()), not just quickedit. This patch would break that.

wesruv’s picture

wesruv CreditAttribution: wesruv at Lullabot commented

Maybe a better approach would be only adding that attribute if the user can edit the entity and then only adding the span if attributes isn't empty. I'll see if I can get a patch with that approach.

wesruv’s picture

wesruv CreditAttribution: wesruv at Lullabot commented

Thanks @cottser!

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez as a volunteer commented 
+++ b/core/modules/node/templates/field--node--title.html.twig
@@ -21,8 +21,14 @@
+{% if user.hasPermission('edit own ' ~ element['#bundle'] ~ ' content') %}

Yeah, and I don't think we'd want this sort of logic in a core template.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
Status: Needs review » Needs work
wesruv’s picture

wesruv CreditAttribution: wesruv at Lullabot commented
Status: Needs work » Needs review

This quickly turned into a quagmire... unless there's a super cool helper function or something to tell me if the current user can edit this field.

In core/modules/quickedit/quickedit.module > quickedit_preprocess_field it's adding a data attribute that fires some JS.

Problem is I'd have to figure out the permission scheme for the parent entity of the field and check if the user has an 'edit' perm.

There doesn't happen to be a 'user can edit this' function or method or something... does there?

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
Status: Needs review » Needs work

"Needs review" is for when the patch needs review, not the approach :)

I don't think this will fly in core templates but probably the most bulletproof way (it can't be tied to quick edit!) would be:

{% set attributes_string %}
  {{ attributes }}
{% endset %}

{% if attributes_string %}
<span{{ attributes_string }}>
...
{% endif %}

Or there is maybe another method on the attribute object that can be called that would make it a bit cleaner. Another issue is adding a toArray.

wesruv’s picture

wesruv CreditAttribution: wesruv at Lullabot commented

Oops! Sorry :P sounded like the right flag at the time.

I was going to add the code in the quickedit_preprocess_field to only add it's data attribute when the user can actually edit, once that was done I was thinking I could detect if attributes actually had anything them.

BUT if the best approach detecting empty attributes won't fly in a core template I think we could close the issue.

Does seem like quick edit shouldn't be adding that attribute unless the field is editable by the user, is that worth filing a bug for?

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented

Does seem like quick edit shouldn't be adding that attribute unless the field is editable by the user, is that worth filing a bug for?

I haven't looked at the code but yes, it sounds like that might be a bug.

wesruv’s picture

wesruv CreditAttribution: wesruv at Lullabot commented
Status: Needs work » Closed (won't fix)
wesruv’s picture

wesruv CreditAttribution: wesruv at Lullabot commented

Going to close this, thanks for that snippet @cottser, going to use that in my own themes.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented

There's probably a cleaner way but that way is performant because the attributes are only rendered once.