#theme => 'links' renders <li class="_"> when the #links array is not associative

This seems wrong.

However, the documentation for what #links is supposed to be is different on links.html.twig vs. template_preprocess_links().

The Twig file says:
links: Links to be output. Each link will have the following elements: ...
(nothing about it being an associative array)

The template_preprocess_links() function says:
links: An associative array of links to be themed. The key for each link is used as its CSS class. ...

So... It is documented at least on the preprocess function that it must be an associative array.

Put a backwards compatibility preprocess into Stable to add that silly class back in but removed it from the templates and removed the documentation of a 'link_key' because that's not a real thing.

Likely will need regression tests with the stuff in the issue summary.

Maybe even a follow-up in 8.1.x to remove this "feature" because these classes can be added in a far better and more explicit way than as the key of the array of links.

Missed the fallback, well that will see if we have tests for this 'feature' if it fails.

I also wondered about this underscore class.

+++ b/core/themes/stable/stable.theme
@@ -0,0 +1,14 @@
+        $variables['links'][$key]['attributes']['class'][] = $key;

I think this need to be $variables['links'][$key]['#attributes']['class'][] = $key;. At least I remember the error from the bot when I have edited the links arrays, but I haved added it to $variables['links'][$key]['link']['#attributes']['class'][] = 'my-example';

Let's see if it works before doing more debugging.

Removed stable.theme and moved into global template_preprocess_links() to fix the many test failures.

@hass thanks for the patches. Interdiff next time would be preferred so we can see what you changed easier.

The reason I moved that to stable.theme was for backwards compatibility in hopes we didn't support adding some seemingly random class on to every li tag in core. With a non-prefixed CSS class like that it could easily cause some nasty CSS conflict by accident.

So the proposal I put forward is to remove this broken and troublesome feature in 8.1.x. I wonder if anybody is relying on this functionality right now, it would be my preference if we could just remove it all together without BC.

Regarding #6 it is actually without the '#' as that is for render array keys and this is not a render array property this is just a list of links with an 'attributes' key. The problem is that it is already an Attribute object. That's why you see the 'Indirect modification of overloaded element' in the test error messages. It's trying to modify a 'class' key on the attribute that doesn't have a class array initialized.

Than we are back to #3 and may need to fix ~1000 lines of tests. I'm fine with this as you really have a good point about these possible random class conflicts. That can cause a lot of troubles for sure. Not sure how to get an interdiff with Eclipse and EGit.

@hass No worries I'm on it. Don't need to fix that many tests only the one test. Just gotta sort out what theme the test is running

\Drupal\system\Tests\Theme\FunctionsTest

An interdiff is really just the changes between the last two commits. So if you locally commit the first patch, then make changes and commit those. An intediff is:
`git show > intediff.txt` or the same thing written a different way `git diff HEAD^ > interdiff.txt`. No idea on egit/eclipse though sorry haven't used that tool.

To be clean we need to fix all... Really. All these failing tests have the LI elements with these random classes. We need to remove it from all asserts to make sure nobody tries to implement future code that works the broken way...

There is only one failing test after I fixed #3. Just away from the computer for tonight, and I added some new test but had to run. Will post tomorrow.

Just pointing out -- see #1, this probably also needs a documentation update?

@jhodgdon totally agree it does thank you.

Edit:
The interdiff is from #3

I'd like to fix this by removing this all together, but talked to @alexpott and @Cottser and it's hard to tell if anybody is relying/using this 'feature'. So best we can do is deprecate it in Stable's BC layer.

Desolé

@jhodgdon Does this cover off #1? I removed the doc about associative arrays and re-wrote it a bit and improved my deprecated notice.

The failing test is a KernalTest which uses core, which now doesn't have those generated classes. There are other tests that check for the same classes like 'CommentOperationsTest', so I fixed the test.

Do you really think this fallback is a good idea? If someone rely on these classes he should change the theme. If he cannot he could add the workaround.

Adding it to stable inherits it to all core themes and also my themes as they rely on classy and classy on stable.

This means I'm not able to get rid of the bug except cloning classy and stable. I think this is a more worse situation than a theoretical and not practical issue that can also be fixed easily. And if we may break a single list in a low usage theme.

This is an acceptable risk. Let's remove this preprocess_links hook, please!

@hass re #24 you are not wrong but we put forth a bit of a contract once we released, stable/classy are API, meaning they don't change and you can rely on that fact. So I'm making due with what we have. There is no reliable way to check if someone is really using this or not in custom code.

Core is fair game to change, Stable/Classy are not.

We are getting rid of the _ bug in the patch. We aren't getting rid of the generated classes because 8.x shipped with that expectation.

If you were using them in your custom/contrib code you'd likely be a bit angry if we changed it on you so I hope you understand why we are doing this for others.

+++ b/core/modules/system/src/Tests/Theme/FunctionsTest.php
@@ -290,6 +290,127 @@ function testLinks() {
+    $expected_links .= '<li data-drupal-link-query="'.$encoded_query.'" data-drupal-link-system-path="router_test/test1"><a href="' . \Drupal::urlGenerator()->generate('router_test.1', $query) . '" data-drupal-link-query="'.$encoded_query.'" data-drupal-link-system-path="router_test/test1">' . Html::escape('Query test route') . '</a></li>';

A space between the dot and the concatenated parts to improve readability
https://www.drupal.org/coding-standards#concat

Updated spaces and comments.

Thanks for the cleanup @madhavvyas . That was mostly a copy paste of the test above except the keys and the expectation classes. Could you post an interdiff so it's easier to see what you've changed?

Provided interdiff

Very much appreciated @madhavvyas thanks for the interdiff.

+++ b/core/themes/stable/stable.theme
@@ -7,8 +7,8 @@
-  // of adding a class based on the associative key can cause CSS class
-  // name conflicts.
+  // of adding a class based on the associative key can cause CSS class name
+  // conflicts.

Ha, reflowed:) I did that on purpose because I'm a designer and "widows" are bad:P But don't care enough to fix that.

And if i used the underscore selector it will break, too. We overcomplicate things for nothing or just a theoretical issue. Fixing bugs should be allowed - also in the themes.

@hass the underscore wasn't documented expected behaviour. And it's not totally theoretical, core uses these classes currently for views dropbutton's delete/edit classes (see the test I modified using core's KernalTestBase, aka core theme)

That's not really a bug though, it's just a potentially problematic/colliding class name arising from a "feature". We have no way of knowing if people are using these classes in their code and even if we shout about this change from the rooftops they likely won't know that what we are changing will break their site. So we just deprecate it and move that class assignment to Stable.

Crosspost with @joelpittet but yeah :)

You will break contrib themes, too. A theme that does not have classy/stable as base and make use of these theoretically used css classes it will also break the theme and require to release a new version with these preprocess function added. All because you moved this to stable theme... I do not get why this should be allowed and changing stable/classy not. In the contrib theme I need to fix both issues. One time i need to add the preprocess function and in the other case I may need to change a css selector. Changing the css selector is much easier than adding preprocess functions.

Sum up - there are several ways how BC will break and moving the logic to stable does not prevent a break in contrib.

With all this in mind we should finally remove it, add it to release notes and forget it. We have done such things in past, too. At least 2 months after D8 release there is only a very very limit number of themes available and usage is low.

RE #37, that is the documented process. The Stable theme is supposed to be stable. The core templates are not. If you want stable markup (including class attributes), use the Stable theme.

Relevant reading: http://cgit.drupalcode.org/drupal/tree/core/themes/stable/README.txt

Most important parts (emphasis mine):

Stable is the default base theme

Warning: Themes that opt out of using Stable as a base theme will need
continuous maintenance as core changes, so only opt out if you are prepared to
keep track of those changes and how they affect your theme.

This would be one of those changes.

Almost there…

1. +++ b/core/modules/system/src/Tests/Theme/FunctionsTest.php
   @@ -290,6 +290,127 @@ function testLinks() {
      /**
   +   * Tests links.html.twig.
   +   */
   

   The docblock should be different to indicate what is different about this test than the one above with the same docblock :)

2. +++ b/core/themes/stable/stable.theme
   @@ -0,0 +1,20 @@
   +<?php
   +
   +use Drupal\Component\Utility\Html;
   +
   +/**
   

   This new file needs a @file docblock.

Thanks @Cottser

Looks good, thanks!

Forgot to mention, because there is no test-only patch I ran the relevant tests locally and things fail as expected. This could probably use a test-only patch and then the full patch uploaded again.

Yeah for clear code and clear documentation, and using Stable for the purpose it was designed for!

Doesn't this mean that code creating a render array with string keys can no-longer rely on those becoming link classes? I'm not clear exactly what the change is there for non-theme code. That's not really a theme API break if so since potentially you could be creating links with classes then writing JavaScript that relies on it. Seems unlikely that someone does this, so possibly we can still change that in 8.1.x with classy handling the bc layer, but needs explanation in the issue summary.

Also issue in general needs a change record.

Moving to 8.1.x since this changes @internal API surface.

@catch re #45 Yes, it really shouldn't rely on that because it is way too easy to have CSS class name conflicts. The non-theme code is there get core closer to what we'd like to have though for BC we kept the auto-generated CSS classes.

There is no API break in the code and it fixes a bug. I don't think this should need a change record as it won't affect anybody's site. If we do actually remove this auto-generated class name, we'd need a change record I'd expect.

Yes, it really shouldn't rely on that because it is way too easy to have CSS class name conflicts.

Right, it should not, but non-module code can rely on this feature to get classes on links currently no? I don't really think #46 answers all the questions in #45 so moving status back.

Sorry, to clarify my thought process for changing it back: it's a bug and it's fixed with tests, therefore 8.0.x eligible. No BC break so no 8.1.x only and no change record. If we could remove that auto-generation of class names we'd totally need a change record and we'd need to expand the scope to cover core's existing views use-cases for the .delete/.edit classes, which I'd love to do but fear it not possible...

Didn't mean to set the status back, I'll let you or someone else.

@catch re #47 I don't understand the question. This leaves the class generation for our API level themes through the hook in Stable.

OK here's the scenario I'm asking about:

- Contrib module A creates an array of links with string keys. It also has it's own CSS and JS files that rely on those keys as classnames.

- Site X uses contrib theme Y which does not inherit from Stable.

- Before patch: the classes get rendered by the theme.

- After patch: the classes do not get rendered by the theme.

If you are Site X, when you update a patch release, you do not expect to also have to update either specific contrib modules or themes due to changes. i.e. we'd be breaking something on your site through no fault of your own, but instead the particular interaction of having module A and theme Y installed.

This might be a 0.0001% chance, but given there are hundreds or thousands of commits per year, and tens or hundreds of thousands of Drupal projects and sites, enough changes like this in enough patch releases and people will stop trusting them.

For minor releases, because of @internal, we assume that contrib or custom modules and themes might have to update for very small changes, and hence sites too - but that's once every six months, not every month.

So it's not an 'API change' because we have Stable and etc. to handle bc for it. But it does change @internal API surface area, which makes it 8.1.x material.

For 8.0.x, we could possibly add that preprocess to system module to be removed again in 8.1.x

@catch that's why we have Stable as API, and the 'Wild west'. Oh but I guess I see the need for a change record now because we told people that if you set base theme: false, you need to keep up with core.

@catch that's why we have Stable as API, and the 'Wild west'.

Yes, so that we can make changes in minor releases. For patch releases we should not break things unnecessarily, which means either not doing this in 8.0.x or ensuring strict BC even for non-stable themes.

@catch it's a bug fix still a bit of a regression because we never had Html::getClass/|clean_class in D7 for this feature which causes the '_' classes. That's at least how I see it.

Added a change record https://www.drupal.org/node/2656648

Right we should remove the underscore classes in 8.0.x in possible, but the array-keys-as-classes we should avoid a functional change in 8.0.x - which would just mean copying the preprocess to system.module in the 8.0.x patch.

@catch does that mean we have to make two change records then?

I kinda wish this issue didn't take me > 25 comments to complete, it's too minor and went through what I thought were the right hoops.

@joelpittet well it's easier with issues like this just to commit them to 8.1.x and forget about 8.0.x, since it's out in a couple of months anyway and no-one will be using 8.0.x after that. Then the issue itself gets committed faster, and it saves all the figuring out of what can and can't go into 8.0.x.

Which is why I moved it there in #45 hoping to do just that...

8.0.x shouldn't need a change notice at all, that's the point of not making the same change there the same way, change records for patch releases should only happen in very exceptional circumstances at this point.

Since the change notice is fine, and that was the only thing that stopped me committing this to 8.1.x straight away, doing that now.

Also marking fixed since this is not cherry-pick-able to 8.0.x given how we're treating semver. If you really want to get it into there to, can re-open for the backport.

Additionally we removed the class generation from system due as this feature of associative index generated class names can cause CSS class name conflicts far too easily.

So this only changed things if array_keys(#links) === [0, 1, 2, 3], not if array_keys(#links) === ['comment-delete', 'comment-reply'], etc.

In fact, we have this scenario in core: comment links. I updated to this commit and can confirm the HTML is unchanged:

<ul class="links inline"><li class="comment-delete"><a href="/en/comment/1/delete" hreflang="en">Delete</a></li><li class="comment-edit"><a href="/en/comment/1/edit" hreflang="en">Edit</a></li><li class="comment-reply"><a href="/en/comment/reply/node/5/comment/1">Reply</a></li></ul>

So, this issue has merely removed the nonsensical case of numeric #links arrays, which then resulted in every <li> getting an class="_" attribute.

Which means this cannot possibly break the scenario in #50.

I shared @catch' concern at first, but upon observing this, am not concerned at all.

@Wim did you test with the Stable theme that has the bc layer? If so #59 is what I'd expect, but the feature is removed for non-Stable themes.

No, the markup shown in #59 is generated by Bartik.

Bartik uses classy as a base theme, which uses Stable as a base theme, so it has the bc layer.

If you test with Stark the classes should not be there.

Right, sorry. This is what is generated with Stark:

<ul class="links inline"><li><a href="/en/comment/1/delete" hreflang="en">Delete</a></li><li><a href="/en/comment/1/edit" hreflang="en">Edit</a></li><li><a href="/en/comment/reply/node/5/comment/1">Reply</a></li></ul>

Whereas before, it used to be:

<ul class="links inline"><li class="comment-delete"><a href="/en/comment/1/delete" hreflang="en">Delete</a></li><li class="comment-edit"><a href="/en/comment/1/edit" hreflang="en">Edit</a></li><li class="comment-reply"><a href="/en/comment/reply/node/5/comment/1">Reply</a></li></ul>

So… my understanding of both the IS and the CR is wrong. #59 is clearly wrong. This is apparently what you need to do now as of this change to get this class:

+++ b/core/modules/system/src/Tests/Theme/FunctionsTest.php
@@ -290,6 +290,127 @@ function testLinks() {
+    $variables['links'][1]['attributes'] = [
+      'class' => ['a/class'],
+    ];

That is not at all mentioned in the CR. Marking NW for that.

@Wim Leers I'm guessing my mention in the CR wasn't clear enough?

Additionally we removed the class generation from system due as this feature of associative index generated class names can cause CSS class name conflicts far too easily.

I've flushed that out further, hopefully well enough
https://www.drupal.org/node/2656648

The before/after examples are still far too suggestive. They suggest it only affects numerically indexed arrays, but it also affects associative arrays. I think including such an example to the CR would make this crystal clear :)

i.e. this:

Providing indexed instead of associative array of class would produce and unexpected '_' underscore class in markup of links.html.twig. Resolved this by moving the class generation to Stable's preprocess.

plus the before/after examples leave me with the impression I don't need to care. But it's actually much more drastic: by default you don't get any class attribute at all anymore. To retain it, you have to use what I cited in #63.

It only effects associative arrays for non-API themes which is what they should expect. (Written clearly in the docs about using base theme: FALSE)

From the docs:

base theme: false

Warning: Themes that opt out of using Stable as a base theme will need
continuous maintenance as core changes, so only opt out if you are prepared to
keep track of those changes and how they affect your theme.

This discussion was more or less what I already tried to say before... I try it again, we should implement a full break change. Things are changing for the most themes already and we still need to note what we changed. This means we could also remove this faulty logic and really fix it.

I think I'm somehow just not communicating this clearly enough.

This is what I meant: https://www.drupal.org/node/2656648/revisions/view/9317876/9318786.

@Wim Leers

This also means that no class attribute is generated anymore by default.

That's not correct, the default comes from Stable and provides the expected classes for the associative array. I think that makes it more confusing

Also

If you want to have class attributes, you now have to specify the value for the class attribute explicitly:

This whole section is not as you expect, that attribute goes on the anchor tag or the span tag, not the LI.

The extra examples seem better though, +1 to those, save for the last one that's not correct.

Removed those two pieces:
https://www.drupal.org/node/2656648/revisions/view/9318786/9318856