Remove all @deprecated code in core themes

looking at the test fails .. confusingly it is comparing two identical strings .. and failing

-'Configure blockEdit view'
+'Configure blockEdit view'

The errors seem unrelated to the changes.

So I am retesting.. but not before I make a minor unrelated correction.

the use html statement is now redundant.

.

Looks it needs to dig into issue with preprocess - all of failures are css class related

+++ b/core/themes/stable/stable.theme
@@ -23,23 +21,6 @@ function stable_library_info_alter(&$libraries, $extension) {
-function stable_preprocess_links(&$variables) {
-  // @deprecated in Drupal 8.0.x and will be removed before 9.0.0. This feature

I bet it not properly deprecated in #2402165: #theme => 'links' renders <li class="_"> when the #links array is not associative

> -'Configure blockEdit view'
> +'Configure blockEdit view'

The trick here is that drupal.org removes the HTML wrapping these things, the difference is going to be in the classes added or whatever this function used to do.

The trick here is that drupal.org removes the HTML wrapping these things

Thanks for providing the solution ...

Bedir++

The differences show up nicely when one tests locally.

My summary might be that perhaps that some tests are too tightly coupled to the expected HTML.
We are in effect re validating the structural of contextual links over and over.

Anyway this will take careful detail work to unpick.

I am not finished, I am just posting as I go.

A few less errors.

I was thinking the same thing...

It will definitely be disruptive .. but we are at the point of incrementing the major release number.

So I would hope that turns the conversation to how do we communicate this to contrib.

In general, It is going to take bespoke fixes... not something we can automate.

Is there a tag for highlighting disruptive changes in the release notes/ migration guides?

CKEditorIntegrationTest now passes locally.

would it be better to move this preprocess into Classy rather than remove it entirely?

-function stable_preprocess_links(&$variables) {
- // @deprecated in Drupal 8.0.x and will be removed before 9.0.0. This feature
- // of adding a class based on the associative key can cause CSS class name
- // conflicts.

I am happy that as we bump the major version number we take the opportunity to stop doing something dangerous.
Doing anything which will potentiality linger for another few years ( or however long the D9 to D10 interval is ), I would speak against

In relation to communicating with contrib .. which does really concern me : the slogan should be

"if you are looking for the the langcode in the classes look at the hreflang instead."

I just don't know the correct forum for that.

With that in mind I have made LanguageSwitchingTest.php pass by focusing on the hreflangs not the classes.

PreviewTest now passes

I am least happy this this change

In conversational english the new xpath search seeks to locate :-

A contextual link DOM structure where the anchor text contains "Add new" and the href contains the string '/filter'

I wanted to use xpaths ends-with() instead of a contains() call for the /filter condition but that is not available in the current mink version.

maybe I should completely specify the href from start to finish.

StatisticsLoggingTest is fixed.

This should all be green now... sorry this one came out in drips and drops.

All resonable pushback .. thanks for the review.

Sure, double quotes it is...

should we just test the raw HTML where possible?

Yes, I am not happy about my cunning plan to "not too tightly" couple the test criteria to the underlying implementation.

There are multiple instances of 'Add new' on the page so I will have to lock things down to href and inner HTML

I will get to it on Sunday.

Ahem finally found the time to work on this...

a) Fixed the double quotes issues.

b) PreviewTest now using the href attribute to lock down the required link

Still not very happy about using :contains(), should we just test the raw HTML where possible?

@longwave .. was that a comment about ALL conversions or the iffy PreviewTest

I see contains() being widely used in our caodebase?

core/modules/views_ui/tests/src/FunctionalJavascript/ViewsListingTest.php

now passes locally

The lines I have altered are already using a mix of single and double quotes so I was forced to escape a least one of them.

I have checked that every altered file now passes lint checks :-

git diff op np --name-only | xargs -n1 php -l

where op and np are the branch names of the old patch and new patch

Ok I think this is good now.

@longwave thank for the review

Before I start, out of context I have looked into the code and the css selector is autoconverted within symfony from

$this->cssSelect('.views-field-operations li.edit a');

to this

descendant-or-self::*[@class and contains(concat(' ', normalize-space(@class), ' '), ' views-field-operations ')]/descendant-or-self::*/li/descendant-or-self::*/a[contains(string(.), 'Edit')]

contains() in this context according to

https://www.w3.org/1999/07/WD-xpath-19990709.html#function-contains

return TRUE when the second string is found in the first. So it does behave as CSS's contains() does.

#28.1) Can this just use assertLink()

I have 2 separable objections :-

In the context of a conversion issue. I think we should try to neither
enhance or degrade the selectively of xpath or css statements.

In behat like language I read ".views.field.operations li a[contains('Edit')] " :-

CSS reads from right to left so

"Of all the links with the text 'Edit'(using wildcards). Return all those in a multi-select
pull down button within the operations section of a table"

Looking at assertLink() I dont think there is a way to preserve that multi select button context
and I a worried about introducing a test instability.

looking the the signature of assertLink()

protected function assertLink($label, $index = 0, $message = '', $group = 'Other') {}

If you set the index to 9 then you are making an assertion that there must be at least 10 matching elements, maybe more.

What I am trying to say it ommiting it and letting the default(0) apply makes me uncomfortable
in senarios in which over time the number of links comes and goes.

For example say there are links with the text 'Edit', 'Editors', and 'Add Editors', 'Add'
that come and go on the page over time. Then selectors like a:contains('Edit') and a:contains('Add') will
maybe match more or less than expected so we can't set the index to a sensible value. We could easily over time add an extra link, then later corrupt the "link
under test" and silently end up with an assertion that falsely remains true.

Looking at the guts of assertLink() it does have one advantage over css contains() -- in xpath land text() matches text exactly.

  protected function assertLink($label, $index = 0, $message = '', $group = 'Other') {
    // Cast MarkupInterface objects to string.
    $label = (string) $label;
    $links = $this->xpath('//a[normalize-space(text())=:label]', [':label' => $label]);
    $message = ($message ? $message : strtr('Link with label %label found.', ['%label' => $label]));
    return $this->assert(isset($links[$index]), $message, $group);
  }
  

Speaking constructively I think the best solution is, is to stop using contains() and copy the xpath/text() example from assertLink().

Alternatively we could copy the code which raised objections in your comments #28.2

$this->assertSame('Edit', $edit_link1->getText());

that snippet invokes the 'named_exact' convention and so also solves the wildcard 'Add Editors' problem.

3) Thank you... I think your suggestion is good. it will make the code cleaner

Finding time to work in this in the run up to Christmas is tricky, and so I am dividing my spare time up into thinking about the problem and creating the patch
I will find time to work on this soon

Did not mean the Christmas break to extend so far into Jan...

So #29 is my response to #28 in word form.

This patch is my response in code form.

+++ b/core/themes/stable/stable.theme
@@ -23,23 +21,6 @@ function stable_library_info_alter(&$libraries, $extension) {
-/**
- * Implements template_preprocess_links().
- */
-function stable_preprocess_links(&$variables) {
-  // @deprecated in Drupal 8.0.x and will be removed before 9.0.0. This feature
-  // of adding a class based on the associative key can cause CSS class name
-  // conflicts.
-  if (!empty($variables['links'])) {
-    foreach ($variables['links'] as $key => $value) {
-      if (!is_numeric($key)) {
-        $class = Html::getClass($key);
-        $variables['links'][$key]['attributes']->addClass($class);
-      }
-    }
-  }
-}
-

We shouldn't remove this from Stable. The comment is from a time before we had decided to create Stable 9 theme so that we wouldn't have to make changes to Stable. We can probably just remove the comment and be done.

Agreed with @lauriii in #32, and this now looks great!

But … this should then probably remove the now irrelevant // @deprecated … comment in stable_preprocess_links().

@Wim Leers, The patch in #33 already removes the // @deprecated ... comment. Or am I missing something?

🤦‍♂️

Nope, you are not missing anything, it was me who was failing to see these lines in the patch 😞 Sorry about that!

Committed 153d8af and pushed to 9.0.x. Thanks!