Custom classes for pager links do not work with Claro theme

Nice work, code looks good to me! I think we could add a test for this by adding a preprocess hook to a test module?

Re-rolling patch against 9.1.x-dev. Kindly review a patch.

I have tested #6 patch it's working fine for me I have attached the screenshot for reference

This issue no longer reproducible in 9.2.x-dev.
Attaching the before patch screenshot for reference.

Applied patch #6 applied successfully

for ref sharing screenshot.....

Thanks @vikashsoni but I don't understand what you tested because your before and after screenshots have the same classes. You have to add code to be able to test this properly. See #4 for guidance.

Please add testing steps when testing and ideally embed the screenshots within the comment so it's easier to see the screenshots. If you install the dreditor browser extension, then you'll see an "Embed" button next to each file.

Note that the testing in #7 and #9 also were not done correctly.

I checked and the patch applies cleanly on 9.3, 9.4, and 10 and should be tested on all 3, so tagging. The formatting of the issue summary isn't using the template so could be cleaned up as well.

Also compared the changes with the other core themes and it's the same approach though Claro is the only one adding additional classes. One other difference is that claro/templates/views/views-mini-pager.html.twig isn't patched and handles the classes differently.

[drupal-9.3.x-dev/9.3.x] [themes]$ grep -r "items.first.attributes|without('href', 'title')" .
./stable/templates/navigation/pager.html.twig:          <a href="{{ items.first.href }}" title="{{ 'Go to first page'|t }}"{{ items.first.attributes|without('href', 'title') }}>
./stable9/templates/navigation/pager.html.twig:          <a href="{{ items.first.href }}" title="{{ 'Go to first page'|t }}"{{ items.first.attributes|without('href', 'title') }}>
./claro/templates/pager.html.twig:          <a href="{{ items.first.href }}" title="{{ 'Go to first page'|t }}"{{ items.first.attributes|without('href', 'title').addClass('pager__link pager__link--action-link') }}>
./classy/templates/navigation/pager.html.twig:          <a href="{{ items.first.href }}" title="{{ 'Go to first page'|t }}"{{ items.first.attributes|without('href', 'title') }}>
./starterkit_theme/templates/navigation/pager.html.twig:          <a href="{{ items.first.href }}" title="{{ 'Go to first page'|t }}"{{ items.first.attributes|without('href', 'title') }}>

[drupal-9.3.x-dev/9.3.x] [themes]$ grep -r "items.previous.attributes|without('href', 'title', 'rel')" .
./stable/templates/navigation/pager.html.twig:          <a href="{{ items.previous.href }}" title="{{ 'Go to previous page'|t }}" rel="prev"{{ items.previous.attributes|without('href', 'title', 'rel') }}>
./stable/templates/views/views-mini-pager.html.twig:          <a href="{{ items.previous.href }}" title="{{ 'Go to previous page'|t }}" rel="prev"{{ items.previous.attributes|without('href', 'title', 'rel') }}>
./stable9/templates/navigation/pager.html.twig:          <a href="{{ items.previous.href }}" title="{{ 'Go to previous page'|t }}" rel="prev"{{ items.previous.attributes|without('href', 'title', 'rel') }}>
./stable9/templates/views/views-mini-pager.html.twig:          <a href="{{ items.previous.href }}" title="{{ 'Go to previous page'|t }}" rel="prev"{{ items.previous.attributes|without('href', 'title', 'rel') }}>
./bartik/templates/classy/views/views-mini-pager.html.twig:          <a href="{{ items.previous.href }}" title="{{ 'Go to previous page'|t }}" rel="prev"{{ items.previous.attributes|without('href', 'title', 'rel') }}>
./claro/templates/pager.html.twig:          <a href="{{ items.previous.href }}" title="{{ 'Go to previous page'|t }}" rel="prev"{{ items.previous.attributes|without('href', 'title', 'rel').addClass('pager__link pager__link--action-link') }}>
./classy/templates/navigation/pager.html.twig:          <a href="{{ items.previous.href }}" title="{{ 'Go to previous page'|t }}" rel="prev"{{ items.previous.attributes|without('href', 'title', 'rel') }}>
./classy/templates/views/views-mini-pager.html.twig:          <a href="{{ items.previous.href }}" title="{{ 'Go to previous page'|t }}" rel="prev"{{ items.previous.attributes|without('href', 'title', 'rel') }}>
./seven/templates/classy/views/views-mini-pager.html.twig:          <a href="{{ items.previous.href }}" title="{{ 'Go to previous page'|t }}" rel="prev"{{ items.previous.attributes|without('href', 'title', 'rel') }}>
./starterkit_theme/templates/navigation/pager.html.twig:          <a href="{{ items.previous.href }}" title="{{ 'Go to previous page'|t }}" rel="prev"{{ items.previous.attributes|without('href', 'title', 'rel') }}>
./starterkit_theme/templates/views/views-mini-pager.html.twig:          <a href="{{ items.previous.href }}" title="{{ 'Go to previous page'|t }}" rel="prev"{{ items.previous.attributes|without('href', 'title', 'rel') }}>

[drupal-9.3.x-dev/9.3.x] [themes]$ grep -r "item.attributes|without('href', 'title')" .
./stable/templates/navigation/pager.html.twig:          <a href="{{ item.href }}" title="{{ title }}"{{ item.attributes|without('href', 'title') }}>
./stable9/templates/navigation/pager.html.twig:          <a href="{{ item.href }}" title="{{ title }}"{{ item.attributes|without('href', 'title') }}>
./claro/templates/pager.html.twig:          <a href="{{ item.href }}" title="{{ title }}"{{ item.attributes|without('href', 'title').addClass(['pager__link', current == key ? ' is-active']) }}>
./classy/templates/navigation/pager.html.twig:          <a href="{{ item.href }}" title="{{ title }}"{{ item.attributes|without('href', 'title') }}>
./starterkit_theme/templates/navigation/pager.html.twig:          <a href="{{ item.href }}" title="{{ title }}"{{ item.attributes|without('href', 'title') }}>

Fixing title.

PSA: You have to add code to be able to test this properly. See #4 for guidance.

Tested the Patch provided in #6
Works Correctly on v9.3.0 v9.4.0
Steps Followed
- Install fresh Drupal setup
- Enable Carlo theme
- Create a view with pager enabled
- Added a template_preprocess_pager hook and added a test class
- Inspect the html code the preprocessed class won't be added.
- Applied the patch
- Cleared cache
- The class is added to the links.
Before Patch

After Patch

Updated the issue summary with steps to reproduce

Thanks @joshua1234511. This looks good! The issue summary could use a bit more of an update but otherwise this looks RTBC.

Updated the issue summary and moving to RTBC based on the above.

This is testing against 9.1.x and failing. Let's get this on 10.0.x.

The patch still applies so I am re-uploading and testing it.

Patch in #6 and #21 are not complete.
Classes are added to all pager links except next page and last page ones.

Here is a fixed patch.

Tested the patch from #22
Against v9.4.0 and v10.0.x
- Enable Carlo theme
- Created a view with pager enabled
- Added a template_preprocess_pager hook and added a test class
- Inspect the html code the preprocessed class won't be added.
- Applied the patch
- Cleared cache
- The class is added to the links.

Based of above manual testing marking this asRTBC

I agree with @acbramley in #4, I think we can add an automated test here.

We already have pager_test_preprocess_pager, and that is adding custom attributes as well as a route for testing a pager.

So it should be a matter of adding 'pager_test' module to ClaroTest and adding a new test method that hits that route and asserts the attribute is output.

See \Drupal\Tests\system\Functional\Pager\PagerTest for reference code that is testing the stark theme.

Took a crack at the test cases

Empty space

1. +++ b/core/tests/Drupal/FunctionalTests/Theme/ClaroTest.php
   @@ -70,4 +71,76 @@ public function testIsUninstallable() {
   +    $logger = $this->container->get('logger.factory')->get('pager_test');
   +    for ($i = 0; $i < 300; $i++) {
   +      $logger->debug($this->randomString());
   +    }
   +    $this->drupalLogin($this->drupalCreateUser([
   +      'access site reports',
   +    ]));
   +
   +    $this->drupalGet('admin/reports/dblog', ['query' => ['page' => 1]]);
   

   pager_test module provides a route with a pager, so you shouldn't need dblog or to insert error messages to test it

2. +++ b/core/tests/Drupal/FunctionalTests/Theme/ClaroTest.php
   @@ -70,4 +71,76 @@ public function testIsUninstallable() {
   +    if (isset($first)) {
   +      $this->assertClass($first, 'pager__item--first', 'Element for first page has .pager__item--first class.');
   +      $this->assertClass($first, 'pager__item--action', 'Element for first page has .pager__item--action class.');
   +      $link = $first->find('css', 'a');
   +      $this->assertClass($link, 'pager__link--action-link', 'Link for element first has .pager__link--action-link class');
   +    }
   +    if (isset($previous)) {
   +      $this->assertClass($previous, 'pager__item--previous', 'Element for previous page has .pager__item--previous class.');
   +      $this->assertClass($first, 'pager__item--action', 'Element for previous page has .pager__item--action class.');
   +      $link = $previous->find('css', 'a');
   +      $this->assertClass($link, 'pager__link--action-link', 'Link for element previous has .pager__link--action-link class');
   +    }
   +    if (isset($next)) {
   +      $this->assertClass($next, 'pager__item--next', 'Element for next page has .pager__item--next class.');
   +      $this->assertClass($first, 'pager__item--action', 'Element for next page has .pager__item--action class.');
   +      $link = $next->find('css', 'a');
   +      $this->assertClass($link, 'pager__link--action-link', 'Link for element next has .pager__link--action-link class');
   +    }
   

   We're not testing for what was causing the issue here, that attributes wasn't being used.

   The pager test module adds an attribute to the pager via preprocess, that's all we need to test is output with claro

@smustgrave pointed out on slack that I'm wrong about the above, the test controller etc and pager test all rely on dblog and seeding log entries.

So ignore item 1 above

Thanks, can we get a test-only version of the patch too (to show that the new test coverage catches the bug)

Also going to rerun the tests on #30 for 10.0.x and 9.5.x

Looking more closely at it I think the patch in #22 is all we need https://www.drupal.org/files/issues/2022-06-09/3123666-22.patch. Classes were printed out but the templates weren't done in best practices so not sure a test is needed.

This should be red/green

The significant element here is a custom class I think

Reviewed and looks good

Let's see if we can get an independent RTBC here given we've worked on a lot of the new changes, I'll ask in #bugsmash

+++ b/core/tests/Drupal/FunctionalTests/Theme/ClaroTest.php
@@ -70,4 +72,30 @@ public function testIsUninstallable() {
+      $this->assertStringContainsString('lizards', $link->getAttribute('class'));

Nit: Is it better to use hasClass() here?

Otherwise looks great.

Nit: Is it better to use hasClass() here?

I didn't know that existed, so yes definitely better.

For #38

As per comment #38, the patch has been re-rolled

The patch has been re-rolled.

Going to mark reviewed since it was a small patch. @arunkumark next time can you please upload an interdiff so we can see the changes. Without it someone has to compare line by line.

@arunkumark, yes, always add a diff or interdiff. Otherwise you are leaving it up to reviewers or a committer to do that work.

@smustgrave, did you review the patch in #43? I suspect you did but you have only commented on the size of the patch.

I did. Appears to be similar to #34 but with hasClass added per #38

seems to be a random unrelated failure

Rerolled against 10.1.x

Moving back to RTBC

Unrelated random failure.

Committed 108a6ef and pushed to 10.1.x. Thanks!