Problem/Motivation

Selecting 'Use path alias' in contextual filter results in an exception. Problem originates from a recent change: #2509300: Path alias UI allows node/1 and /node/1 as system path then fatals

Steps to reproduce:

  1. Add a new view (default settings)
  2. Add contextual filter 'Content: Node ID' - Settings:
    • Default actions : Provide default value
    • Type : Raw value from URL
    • Use path alias : checked
  3. Click "Apply"

Results in the following:

Uncaught PHP Exception InvalidArgumentException: "Source path admin/structure/views/view/articles/preview/rest_export_1 has to start with a slash." at c:\...\www\...\core\lib\Drupal\Core\Path\AliasManager.php line 194

Saving the view while the filter is present will also produce the error.

Proposed resolution

Change trim() to rtrim() in Raw::getArgument (default argument plugin).

Will require some refactoring to keep the path component index syncing up with the proper arguments.

Add new test that checks for NULL or nonexistent path arguments.

Remaining tasks

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#26 source_path_has_to-2569117-26.patch3.62 KBcilefen
#20 interdiff.txt1 KBothermachines
#20 drupal_core-2569117-18.patch3.6 KBothermachines
#17 drupal_core-2569117-17-TEST-ONLY.patch2.55 KBLendude
#16 drupal_core-2569117-16.patch3.43 KBothermachines
#16 interdiff.txt3.36 KBothermachines
#14 interdiff.txt2.4 KBothermachines
#14 drupal_core-2569117-14.patch3.09 KBothermachines
#4 drupal_core-2569117-4.patch705 bytesothermachines
#2 drupal_core-2569117-contextual_filter_exception-1.patch1.11 KBothermachines
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

othermachines created an issue. See original summary.

othermachines’s picture

othermachines CreditAttribution: othermachines commented
Status: Active » Needs review
FileSize
drupal_core-2569117-contextual_filter_exception-1.patch1.11 KB

Status: Needs review » Needs work

The last submitted patch, 2: drupal_core-2569117-contextual_filter_exception-1.patch, failed testing.

othermachines’s picture

othermachines CreditAttribution: othermachines commented
Status: Needs work » Needs review
FileSize
drupal_core-2569117-4.patch705 bytes

I'm a creature of habit.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Issue tags: +Needs tests

Thank for filing the issue and providing a fix!

That indeed totally makes sense, but we should better add some tests in order to ensure it doesn't happen in the future anymore.

We have a test in core/modules/views/tests/src/Unit/Plugin/argument_default/RawTest.php so we might could just expand that one

Status: Needs review » Needs work

The last submitted patch, 4: drupal_core-2569117-4.patch, failed testing.

The last submitted patch, 2: drupal_core-2569117-contextual_filter_exception-1.patch, failed testing.

The last submitted patch, 4: drupal_core-2569117-4.patch, failed testing.

othermachines’s picture

othermachines CreditAttribution: othermachines commented

@dawehner You're welcome. Was hoping it would be fast and easy but I'm a bit held up due to #2294731 Simpletest fails to run PHPUnit on Windows and in extracted tarballs of Drupal. Maybe need to resort to running from command line as you suggest but will need to install, etc. There are never enough hours in the day...

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

Maybe need to resort to running from command line as you suggest but will need to install, etc. There are never enough hours in the day...

Well, true. I think running the following on the command line should be enough:

php core/vendor/bin/phpunit -c core core/modules/views/tests/src/Unit/Plugin/argument_default/RawTest.php
othermachines’s picture

othermachines CreditAttribution: othermachines commented

Or what you said, even. :)

For the record, I was able to run it successfully on Windows with:

$ cd core
$ php ./vendor/phpunit/phpunit/phpunit modules/views/tests/src/Unit/Plugin/argument_default/RawTest.php

-c wasn't getting me anywhere. Should I be concerned about excluding it?

Thanks for your help!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

Oh well, the -c core is just needed in case you don't do the cd core before.

othermachines’s picture

othermachines CreditAttribution: othermachines commented

Ah sorry, that was confusing. I was just referring to the symlink not working in Windows (according to handbook page). But what -c core does now makes more sense. Thanks again.

othermachines’s picture

othermachines CreditAttribution: othermachines commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
drupal_core-2569117-14.patch3.09 KB
interdiff.txt2.4 KB

First stab at a PHPUnit test. Let's hope this works. Thanks to @dawehner for helping me out here.

Revisions to testGetArgument():

  1. Checks the return value of getArgument() for three path arguments -- [ ] / [ test ] / [ example ] -- instead of two. In the first instance 'use_alias' is FALSE (Use path alias is NOT selected).
  2. Modifies the parameter passed to getAliasByPath() as having a leading slash while expecting same as the return value.
  3. Again checks path arguments in the return value of getArgument(), but this time 'use_alias' is TRUE (Use path alias is selected).
othermachines’s picture

othermachines CreditAttribution: othermachines commented
Status: Needs review » Needs work

Yeah, no that's not gonna work. It occurred to me in the light of day that those URL indexes aren't just arbitrarily set for the purpose of testing. Back to it then...

othermachines’s picture

othermachines CreditAttribution: othermachines commented
Status: Needs work » Needs review
FileSize
interdiff.txt3.36 KB
drupal_core-2569117-16.patch3.43 KB

Ignore the last set of uploads. The diff is against the first patch.

New changes to getArgument():

  1. Adds an array_shift() to lop off the empty first element to ensure our function returns the same value as before based on path component index.

Changes to testGetArgument():

  1. Adds new checks that compare the return value of getArgument() when $options['index'] is NULL or path argument doesn't exist. In the first instance 'use_alias' is FALSE (Use path alias is NOT selected).
  2. Modifies the parameter passed to getAliasByPath() as having a leading slash while expecting same as the return value.
  3. Same as #1, but this time 'use_alias' is TRUE (Use path alias is selected).

I ran testGetArgument() with additions in #1 and #3 and no changes to getArgument() to confirm the expected return values remain consistent:

Value of $options['index']  |  Expected return value
-----
NULL (not set)              |  NULL
0                           |  'test'
1                           |  'example'
2                           |  NULL
Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer commented
FileSize
drupal_core-2569117-17-TEST-ONLY.patch2.55 KB

Manually tested this. Could reproduce the issue with the steps described and the proposed patch solves the issue.

Always a good idea to have a failing test-only patch to highlight the problem, so I'll add that.

Couple of nitpicks:

  1. +++ b/core/modules/views/src/Plugin/views/argument_default/Raw.php
@@ -103,11 +103,13 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
+    // Don't trim the leading slash.

    Maybe also explain why not? 

  2. +++ b/core/modules/views/src/Plugin/views/argument_default/Raw.php
@@ -103,11 +103,13 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
+    array_shift($args);

    Maybe add a comment along the lines of what was said in #16.1?

Looks good otherwise.

Status: Needs review » Needs work

The last submitted patch, 17: drupal_core-2569117-17-TEST-ONLY.patch, failed testing.

The last submitted patch, 17: drupal_core-2569117-17-TEST-ONLY.patch, failed testing.

othermachines’s picture

othermachines CreditAttribution: othermachines commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
drupal_core-2569117-18.patch3.6 KB
interdiff.txt1 KB

@Lendude Thanks for the review and the test-only patch. (I've noticed the practice but I'm not clear on how it works.) Here's a revision based on your comments. Also updated issue with a couple more details.

othermachines’s picture

othermachines CreditAttribution: othermachines commented
Issue summary: View changes

Oops - broken link.

othermachines’s picture

othermachines CreditAttribution: othermachines commented
Issue summary: View changes

gah - sorry. It's too early.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

@othermachines you can link to other issues using square brackets, # and the issue number. Then you get the nice status colouring and strikethrough when fixed.

The test-only patches are for showing that it's an actual bug in HEAD. And then your combined test/fix patch show that you fixed the bug exposed by the test-only patch. Bit of reading on the how and why.

Patch looks go to me now.

othermachines’s picture

othermachines CreditAttribution: othermachines commented

@Lendude Really? So I don't have to type <a href> anymore? Thank god. In a perfect world there would only be markdown.

Thanks very much for the info!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 20: drupal_core-2569117-18.patch, failed testing.

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Needs work » Needs review
FileSize
source_path_has_to-2569117-26.patch3.62 KB

This is a reroll.

othermachines’s picture

othermachines CreditAttribution: othermachines commented

Confirming patch in #26 is good - no error, unit test passes.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Straight reroll so back to RTBC

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Issue tags: +rc target triage

An exception sounds like a potential RC target

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Priority: Normal » Major
Issue tags: -rc target triage +rc target

Discussed with @effulgentsia. This is a pretty common usecase and it's basically completely broken, so tagging as an RC target.

  • catch committed af81303 on 8.0.x 
    Issue #2569117 by othermachines, Lendude, cilefen: "Source path ... has...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed af81303 on 8.1.x 
    Issue #2569117 by othermachines, Lendude, cilefen: "Source path ... has...

Status: Fixed » Closed (fixed)

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