Problem/Motivation

Since Drupal 10.2, the "More" link in views does not respect the default arguments anymore. This leads to instances where the "More" link points to wrong and/or inaccessible pages.

Steps to reproduce

1. Install Drupal 10.1.8 and Drupal 10.2.3 in the standard configuration. In both instances, follow the following steps:
2. Install the forum module
3. Import the attached view and the attached block config
4. Navigate to /forum/1
5. Inspect the "More" link in the views block in the sidebar
→ In Drupal <10.2, the link is correctly pointing to /forums/1/page
→ Since Drupal 10.2, the link is pointing to /forums/all/page (→ the "1" is dropped as an argument)

Proposed resolution

I think that #981870 introduced this regression. In particular, in the ViewExecutable.php the following code in the getUrl() method was changed from

          if (!empty($argument->is_default) && !empty($argument->options['default_argument_skip_url'])) {
            unset($args[$position]);
          }

to

          if (!empty($argument->is_default)) {
            unset($args[$position]);
          }

This way, as far as I understand it, default arguments are now always skipped by default. I'm not sure if that behavior was intended. My understanding is that the opposite should be the case, i.e., the default arguments should never be skipped.

If my understanding is wrong, I think the change record is highly misleading since it doesn't imply a necessity to re-configure any views, especially not views that did not even use the "Skip default argument for view URL" function.

Remaining tasks

- Write actual fix

Issue fork drupal-3422710

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nico.b created an issue. See original summary.

nico.b’s picture

Issue summary: View changes
nico.b’s picture

Status: Active » Needs review

The fix might be easy. Before #981870, the code part mentioned above did only run if default_argument_skip_url was set to TRUE. Since that setting has been entirely removed, I believe that we can completely delete that code section (instead of always executing it, as it is now).

Created an MR.

smustgrave’s picture

Version: 10.2.x-dev » 11.x-dev
Status: Needs review » Needs work
Issue tags: +Needs tests

Thanks for reporting,

Will need a failing test to show the issue.

MR target should be 11.x as the current development branch.

nico.b’s picture

nico.b’s picture

FileSize
8.49 KB
smustgrave’s picture

FYI no need to upload test-only patches. That feature is in gitlab now.

nico.b’s picture

Status: Needs work » Needs review

Thanks for the info, @smustgrave. Then the test documentation is probably not up-to-date yet ;)

I've added a test case in the MR and changed the target to 11.x.

There seems to be one test failure, but it seems totally unrelated - not sure why that is coming up.

nico.b’s picture

Ok, seems like there was indeed an issue in 11.x making the test fail. I've merged a fix from 11.x and now all tests are passing again :)

Also the test-only changes show the failing test case, so this issue should now be ready for review.

adwivedi008’s picture

Status: Needs review » Reviewed & tested by the community

MR at #09 seems good to me, it also passes test cases over the pipeline.
Moving the issue to RTBC.

@smustgrave Please review and share if any other change is required.

alexpott’s picture

I'm pretty sure this issue is correct and the way #981870: Skip default argument for view URL does not work and never has so it can be removed was implemented was incorrect. But before we merge this it'd be great if the people who work on that issue add a +1.

I think the fact that no test coverage is changed here proves that core actually had no test coverage of what they were changing in that issue.

  • longwave committed 01176760 on 10.2.x
    Issue #3422710 by nico.b, smustgrave, adwivedi008, alexpott: "More" link...

  • longwave committed b2450c97 on 10.3.x
    Issue #3422710 by nico.b, smustgrave, adwivedi008, alexpott: "More" link...

  • longwave committed 49063a57 on 11.x
    Issue #3422710 by nico.b, smustgrave, adwivedi008, alexpott: "More" link...
longwave’s picture

Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs tests

Whoops, in the original issue we got the condition the wrong way round.

Committed and pushed to 11.x and 10.3.x, backported to 10.2.x as an eligible bug fix. Thanks!

Status: Fixed » Closed (fixed)

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