Problem/Motivation

In Twig debug comments, Theme suggestions should be listed with the most-specific ones first and more-generic ones later. When a View has a "more link" added to it, the suggestions for its container are in the reverse order from what they should be.

<!-- FILE NAME SUGGESTIONS:
   * container--more-link.html.twig
   * container--more-link--frontpage.html.twig
   * container--more-link--page.html.twig
   * container--more-link--frontpage--page.html.twig
   * container--more-link--default.html.twig
   * container--more-link--page-1.html.twig
   * container--more-link--frontpage--page-1.html.twig
   x container.html.twig
-->

The container--more-link--frontpage--page-1.html.twig is more specific than container--more-link.html.twig, so it should come before it in the list.

This is caused because of a bug in views_theme_suggestions_container_alter() which uses ViewExecutable::buildThemeFunctions() incorrectly. ViewExecutable::buildThemeFunctions() returns theme suggestions in the same order as needed for a #theme entry in a render array. But all the theme suggestions hooks require suggestions in the opposite order (to make adding more-specific suggestions as easy as appending to an array.)

Steps to reproduce

Edit the frontpage view (/admin/structure/views/view/frontpage) and turn on the "more link" option. Then visit the /node page and look at the new "more" link at the bottom of the node list.

Proposed resolution

Perform an array_reverse() on the results of ViewExecutable::buildThemeFunctions() before using it in views_theme_suggestions_container_alter().

User interface changes

None.

API changes

None.

Release notes snippet

The Views "more link" now lists theme suggestions in the proper order. For example, a container--more-link--frontpage.html.twig template will now override a container--more-link.html.twig template.

CommentFileSizeAuthor
#14 3188122-14.test-only.txt7.75 KBdww
#13 after_patch.png218.93 KBsmustgrave
#13 before_patch.png83.33 KBsmustgrave
#12 3188122-after_patch.png39.99 KBabhijith s

Issue fork drupal-3188122

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

JohnAlbin created an issue. See original summary.

JohnAlbin opened merge request !152

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented
Status: Active » Needs review
lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs review » Needs work

Did a little digging a to why we are doing this at all, but seems to stem from #2036195: Remove views-more.html.twig and replace with #type link render arrays and trying to maintain some sort of feature parity when the views specific more template was removed. I think.....

Looks good, nice test coverage. One thing, the new test isn't using the browser, so can we do that in a kernel test please? Maybe add it to \Drupal\Tests\views\Kernel\Plugin\DisplayPageTest since that already does some read more testing?

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented

Looks good, nice test coverage. One thing, the new test isn't using the browser, so can we do that in a kernel test please?

Thanks for the review! And I'll start working on refactoring the tests to use kernal tests instead.

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented
Status: Needs work » Needs review

OK. I've moved the tests to ViewsTemplateTest which extends ViewsKernelTestBase.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

dww’s picture

dww
we/he/they
commented
Issue tags: +Bug Smash Initiative

Came upon this bug while working on a client site today. Excited to see this looks basically ready and done. Tagging to be smashed. Hope to have time to review this week. 🤞

steveoriol’s picture

steveoriol
Location Grenoble 🇫🇷
commented

This MR can not be not apply if the MR from #2118743: Twig debug output does not display all suggestions when an array of theme hooks is passed to #theme is used.

abhijith s’s picture

abhijith s commented
StatusFileSize
new 3188122-after_patch.png39.99 KB

Applied MR 152 on 9.5.x and fixes the issue.The template suggestion hierarchy is appearing correctly after applying this patch.

After patch:
after

RTBC +1

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new before_patch.png83.33 KB
new after_patch.png218.93 KB

Tested the PR and confirmed it works as described

dww’s picture

dww
we/he/they
commented
StatusFileSize
new 3188122-14.test-only.txt7.75 KB

Agreed, this looks ready:

  1. The summary is clear and accurate.
  2. There's a release note snippet if we wanted it (though I doubt we'll use it).
  3. The MR looks good, does what it says, and includes a test.
  4. I checked out the MR branch locally, reverted the fix, and the test failed as expected. The output (attached) is a bit clumsy since it includes the entire text of the response, but that's how assertStringContainsString() works, and that seems to make the most sense for this test.
  5. I don't think this needs a change record.

RTBC++ 😉

Thanks!
-Derek

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs work

The MR is testing on 9.3. That needs to be updated to 9.5. Or maybe switch to a patch so we can test on multiple versions easily.

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented
Version: 9.5.x-dev » 10.1.x-dev
Assigned: Unassigned » johnalbin

I'll try to get this rebased on 10.1.x during Drupalcon Prague this week.

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented
Assigned: johnalbin » Unassigned
Status: Needs work » Needs review

I merged 10.1.x into this branch and I didn't have to fix any merge conflicts. Hopefully, the tests will pass and this will re ready to RTBC again.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

This is was on my list of tickets to review. I just rebased the branch.

Also tested out manually and confirmed it was working.

larowlan made their first commit to this issue’s fork.

larowlan opened merge request !2992

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Rebased the 10.1.x branch and made a 9.5.x version

If they both pass I'll commit this. Ideally the 9.5.x version was done before it was RTBC (it was asked for in #15) but 🤷

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs work

Setting to NW because of failing tests.

larowlan credited B-Prod.

larowlan credited levmyshkin.

larowlan credited raman.b.

larowlan credited ranjith_kumar_k_u.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Closed related issue as a Duplicate of this one

Adding credit from the other one

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented

The 9.5 version only seems to contain the test changes? So not too shocking that it fails ;)

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Sounds like I messed that up, will look today

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs work » Needs review

Added all three commits this time, if someone else is willing to RTBC this I still think I'm eligible to commit it as all I'm doing is git commands

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Changes look good.

Also the MR does apply for 10.1.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Removing credit for my failed branch work

  • larowlan committed caa705a7 on 10.1.x authored by JohnAlbin 
    Issue #3188122 by JohnAlbin, larowlan, smustgrave, dww, Abhijith S, B-...

  • larowlan committed 9266e0ea on 10.0.x authored by JohnAlbin 
    Issue #3188122 by JohnAlbin, larowlan, smustgrave, dww, Abhijith S, B-...

  • larowlan committed 74d974a0 on 9.5.x authored by JohnAlbin 
    Issue #3188122 by JohnAlbin, larowlan, smustgrave, dww, Abhijith S, B-...

larowlan closed merge request !2992

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Version: 10.1.x-dev » 9.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed to 10.1.x and backported to 10.0.x and 9.5.x

Thanks

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.