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
Comment | File | Size | Author |
---|
Issue fork drupal-3422710
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:
Comments
Comment #3
nico.b CreditAttribution: nico.b as a volunteer commentedComment #4
nico.b CreditAttribution: nico.b as a volunteer commentedThe 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.
Comment #5
smustgrave CreditAttribution: smustgrave commentedThanks for reporting,
Will need a failing test to show the issue.
MR target should be 11.x as the current development branch.
Comment #6
nico.b CreditAttribution: nico.b as a volunteer commentedComment #7
nico.b CreditAttribution: nico.b as a volunteer commentedComment #8
smustgrave CreditAttribution: smustgrave commentedFYI no need to upload test-only patches. That feature is in gitlab now.
Comment #9
nico.b CreditAttribution: nico.b as a volunteer commentedThanks 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.
Comment #10
nico.b CreditAttribution: nico.b as a volunteer commentedOk, 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.
Comment #11
adwivedi008 CreditAttribution: adwivedi008 at OpenSense Labs for DrupalFit commentedMR 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.
Comment #12
alexpottI'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.
Comment #16
longwaveWhoops, 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!