Strange duplicate IDs during node preview and when a node is displayed multiple times on a page

Adding tag. :)

Right now, Drupal 7's node.tpl has:

<div id="node-<?php print $node->nid; ?>" class="<?php print $classes ?> clearfix">

We could add a $id variable and change it to:
<div <?php if ($id) { print 'id="' . $id . '"'; } ?> class="<?php print $classes; ?> clearfix">

And then ensure that $id is unique in the background.

Alternatively, we could move node-NID from an ID to a class, so it would just be added to $classes.

So, which of those 2 solutions do we want to go with?

I think there may be times where a node may be displayed more than once on a page. For example, when you're displaying both the teaser and the full node on the preview page. as john cross posted at #405270: Strange duplicate IDs during node preview and when a node is displayed multiple times on a page

So, i think it makes the most since to move NID to a class. I think it may also be good practice to make the change to node.tpl as well.

I think there may be times where a node may be displayed more than once on a page.

I agree with @rcross in #4 that there are times when a node may be displayed multiple times on a page and therefore having duplicate CSS ids is invalid code. I think the cleanest thing to do would be to remove the CSS id and move the node nid into a CSS class. Though backporting that may be difficult. I can't think of a way to do it that wouldn't break a lot of folks' existing code.

Marked #1806524: Remove id selector from node.twig as it can be non-unique as duplicate.

Important from that duplicate issue is:

jwilson3:

The problem with this is that the ID is the only valid use of URL fragment navigation in HTML5, because anchor tags with the name attribute is now invalid in HTML5. This is a pattern used by Drupal core no less for navigating to comments on a page, so it might be argued that this should not be removed from nodes either.

I think to solve it we could:

* Remove the #id attribute for teaser display
* Add a class attribute with node-[nid] always

The argument is that potentially the same node would appear on the page more than once, but its just an assumption that they would be different view modes Examples given were the preview (where full node and teaser are show). There are other cases where potentially two view modes might be shown (eg, sideshow and node list).

Instead of code that targets just the teaser display to remove the id, might we instead add the view-mode to the id, for all except the default view mode? This would more widely cover the use case of a node listing appearing more than once on a page, but I still don't think it actually gets at the heart of the issue.

IMO, the only proper solution is to track a registry of nodes on the page and build a method that generates a unique id, similar to how form-api works when you add the search form to the page twice, for example.

Whatever ends up happening, I would also propose moving the logic for the id, to a preprocess function, and to introduce the Attribute object here.

I partially agree with #5, in that anything short of a registry of node ids would have a much higher chance of breaking peoples existing designs for a backport to D7.

Presumably just something like this, no?

There's probably no way to backport this in preprocess, though, since for other themes we'd start seeing two "id" attributes on the same node. (I think to backport it at all, the only possibility to consider would be fixing it directly in the template.)

Nice! This solution is very clean -- putting the id into the Attributes array in preprocess and removing it completely from the template, and letting Drupal do its job -- no themer should be needing to change the id attribute in a template anyway.

#9: That is a nice approach :)

feel free to overrule me, if someone finds something wrong with #9, but it seems like a clear and easy win to me.

The only potential thing we are *losing* in this patch is code beautification: the id now comes after a potentially long list of classes, harder to find/read/reference by humans. I don't think this constitutes further work on this solution. If the themer deems this important enough, they can override/implement their own node.tlp.php (or Twig as the case may be) to print out the id before the class, using the syntax provided in the template for class as a guide. Thoughts?

Easy to fix:

Just do:

<article id="<? print $attributes['id']; ?>" class="<? print $attributes['class']; ?>"<? print $attributes: ?>>

Then its no problem anymore ...

Probably we could rearrange things in the preprocess function so that 'id' is the first thing added to the 'attributes' array?

Then, if the class="..." stuff that's currently hardcoded in the template ever gets moved to preprocess in another issue (which it probably should?), the ID would be first in HTML again.

Oh.. or #14 works too :)

#15: No, themers need to be able to add classes easily, such the class="{{ attributes.class }} clearfix" will definitely need to stay. (writing twig as thats faster to type)

I think making the id explicit is better, because if a themer wants to change this, they can. (not that they should, but its crystal clear what is happening).

Would love a re-roll of #9 with #14 included.

Tentatively setting to NW to get that change in, then can be back to RTBC :-).

Sorry, beforehand. Yes, this is a total side note, but interestingly enough, I've been theming Drupal for a while now, but I've never needed to add a class to an object where classes were already present, but only to objects where Drupal doesn't provide any class at all.... *ahem* forms. Drupal is really good at providing all the necessary classes out of the box that any web developer / themer worth his grain of salt could figure out a proper css directive chain to target the item needed without additional classes.

I personally side with #15, that the class="{{ attribute.class }}" stuff could go away, and be documented once or twice in various places around the code on how to add classes in template files. It almost seems *cleaner* to show people how they could add a class in a preprocess function instead of overriding a template file just to add a classname.

In summary, I'm not trying to derail here -- so if this input is useful, maybe we should just open up a followup issue about this topic.

UPDATE; The unwritten assumption I'm making is that if you know enough to figure out how to override a template file in a custom theme, you also know enough to read documentation on how to override and extend the attributes classes.

I can see both sides of that argument myself, but given that "id" does appear in the template files currently, I suppose the simplest change this issue can make is just to leave it there.

So, here's a patch along the lines of @Fabianx's suggestion in #14.

#18: @jwilson3: This is not true for most non programmer themers, but I'll just point you to the sandbox issue, where this has been debated already:

#19: Thanks a lot @David_Rothstein. Back to RTBC now.

#19 looks good for me
+1 RTBC

diff --git a/sites/default/default.settings.php b/sites/default/default.settings.php
old mode 100755
new mode 100644

Oops, this wasn't supposed to be here. (I have an installation script that changes file permissions on things in sites/default and it accidentally "fixed" this and slipped into the patch.)

There's actually an issue to fix that at #1830330: Make default.settings.php not executable already, so to avoid confusion I'm uploading a new patch file that removes those changes from this one.

Leaving at RTBC since there are no changes to the rest.

#18 @Fabianx, you missed the link... It would be much appreciated tho. ;) Also, personal shoutout, thank you, and congratulations on your hard work getting Twig in. I'll be celebrating this evening. nice forward progress.

#23: Thanks a lot! Here is the link: http://drupal.org/node/1808254

#22: Still RTBC - would love to see this in :-).

#22: node-unique-id-405270-22.patch queued for re-testing.

Needs re-roll ... and should also change the twig template ...

Will this be backported to D7?

If not, would it cause any problems if I replace

print $node->nid;

with

print drupal_html_id($node->nid);

in node.tpl.php of my theme.

It's a simpler fix but seemed to work fine when I tested it.

(Merging "node system" and "node.module" components for 8.x; disregard.)

The id attribute was removed from the node template in #2004252: node.html.twig template