id attribute block.html.twig and node.html.twig

However, it's still useful to be able to target blocks independently

I assume you are talking about stark. While it is true that this is useful from a CSS perspective, this isn't going to be our approach to stark in D8. Stark is to be as clean as possible, while Bartik will provide these sorts of selectors. You might want to read the BADCamp sprint summary for some background on this decision: http://groups.drupal.org/node/265858

I'll put Bartik on the agenda for the next Twig Google hangout, details are on the Sandbox node.

There is a very similar TODO in node.html.twig:

might be a good idea to remove the id attribute, because if that gets rendered twice on a page this is invalid CSS for example: two lists in different view modes.

I tried to search the Drupal issue queue, because i distinctly remember discussing this earlier in a separate thread, but I couldn't find it. my search fu is weak.

Regarding the patch above, I believe we should separate the removal of wrapper div from this issue, because it introduces what I believe to be an inconsistency (note the node.html.twig also has an h2 and a wrapper div, that no one has suggested should be removed). From a quick search, only zen theme has discussed this: #1361592: Remove div.content from block.tpl, so I've created #1914164: Remove div.content from block.html.twig and node.html.twig for that discussion.

I've committed links to these issues so that anyone reading the todo's in the code will know where to go to find the discussion.

Removed the id from the twig template and the todo.

Didn't remove the outer container as that is from #1914164: Remove div.content from block.html.twig and node.html.twig

Also, this won't remove the actual id attribute because it's still in the {{ attributes }} supposedly coming from preprocess_container which I need to trace to find out how that is all happening later.

committed in 35c5f1c

Also, this won't remove the actual id attribute because it's still in the {{ attributes }}

Also, the docs do say attribute.id is guaranteed unique.

See this is why this entire issue and patch doesn't make sense. If Drupal is still providing the id, then we should leave it there, until the underlying issue is resolved in core.

Also, did you commit the change for node.twig as well. To me this issue is not resolved, just now its more obscured ;)

I don't have a problem with the usage of {{ attributes }} in Stark (i.e. core default markup). If we want to break out id separately, let's do it in Bartik. The only reason for this is visibility for folks using the template as part of a base theme, and we've decided that Bartik is a better candidate for this mission than Stark.

If we want to break out id separately, let's do it in Bartik [which] is a better candidate for [folks using the template as part of a base theme] than Stark.

By that line of reasoning, attributes.class should also be removed from templates in Stark too, no?

I didn't commit the change for node, I think someone else did because I didn't see it in the code.

Removal of attributes.class seems like good reasoning but may be a can of worms too but still make sense to not have it in stark either.

Can we please move this discussion to the relevant core issues or create new one(s) if need be? Both of these templates already have pending core conversion issues so making these commits in the sandbox won't change things too much.

#1898034: block.module - Convert PHPTemplate templates to Twig
#1898432: node.module - Convert PHPTemplate templates to Twig

Thanks!

I am moving this issue back to Drupal core from the (now deleted) sandbox project used for the development of the theme engine that uses Twig.

I apologize for bumping this issue.

Drupal core as of 10.2 (i.e., the 11.x branch) now has clean_unique_id Twig filter, thanks to #3115445: Add a new clean_unique_id Twig filter for Html::getUniqueId. Which might make change the proposed resolution and patches.