Problem/Motivation

See https://qa.drupal.org/pifr/test/883523

* Drupal\views_ui\Tests\DisplayPathTest (97 pass(es), 2 fail(s), and 0 exception(s))
   - [fail] [Fatal error] "[09-Oct-2014 12:26:18 UTC] Uncaught PHP Exception InvalidArgumentException: "The URI "base://(:;2&+h^" is invalid. You must use a valid URI scheme. Use base:// for a path, e.g., to a Drupal file that needs the base path. Do not use this for internal paths controlled by Drupal." at /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Url.php line 207
" in Unknown on line 0 of Unknown.
   - [fail] [Fatal error] "[09-Oct-2014 12:26:18 UTC] Uncaught PHP Exception InvalidArgumentException: "The URI "base://(:;2&+h^" is invalid. You must use a valid URI scheme. Use base:// for a path, e.g., to a Drupal file that needs the base path. Do not use this for internal paths controlled by Drupal." at /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Url.php line 207
" in Unknown on line 0 of Unknown.

Proposed resolution

  • Don't allow ? as part of the path
  • Don't allow an empty path but a query

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#22 interdiff.txt633 bytesdawehner
#22 2353347-22.patch2.01 KBdawehner
#18 interdiff.txt645 bytesdawehner
#18 2353347-18.patch2.03 KBdawehner
#16 2353347-16.patch1.95 KBdawehner
#8 interdiff-2353347-2-8.txt1.94 KBzaporylie
#8 2353347-8.patch2.75 KBzaporylie
#6 interdiff-2353347-2-6.txt1.89 KBzaporylie
#6 2353347-6.patch2.76 KBzaporylie
#2 interdiff.txt809 bytesdawehner
#2 2353347-2.patch2.74 KBdawehner
#1 2353347-1.patch2.72 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Priority: Critical » Major
Status: Active » Needs review
FileSize
2353347-1.patch2.72 KB

There we go.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
2353347-2.patch2.74 KB
interdiff.txt809 bytes

Let's use a better variable name.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Priority: Major » Critical
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Component: views.module » views_ui.module
Issue summary: View changes
Related issues: +#2353457: Random test failure in Drupal\views_ui\Tests\DisplayPathTest

Marked #2353457: Random test failure in Drupal\views_ui\Tests\DisplayPathTest duplicate.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

Only nitpicks:

  1. +++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
@@ -449,6 +450,15 @@ protected function validatePath($path) {
+    $parsed_url = UrlHelper::parse($path);

    I think I know why this uses UrlHelper::parse() rather than parse_url(), but shouldn't we document that very briefly? 

  2. +++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
@@ -449,6 +450,15 @@ protected function validatePath($path) {
+      $errors[] = $this->t('No path query allowed.');

    s/path query/query/
    ? 

  3. +++ b/core/modules/views_ui/src/Tests/DisplayPathTest.php
@@ -92,8 +92,24 @@ public function testMenuOptions() {
+    // Add an invalid path (empty just fragment).

    s/empty just fragment/only fragment/ 

  4. +++ b/core/modules/views_ui/src/Tests/DisplayPathTest.php
@@ -92,8 +92,24 @@ public function testMenuOptions() {
+    // Add a path with a query.
...
+    // Add a path with just a query.

    s/a path/an invalid path/ 

  5. +++ b/core/modules/views_ui/src/Tests/DisplayPathTest.php
@@ -92,8 +92,24 @@ public function testMenuOptions() {
+    // Provide a valid path string.

    s/valid/random, valid/

zaporylie’s picture

zaporylie
he/him
Primary language Polish
CreditAttribution: zaporylie commented
FileSize
2353347-6.patch2.76 KB
interdiff-2353347-2-6.txt1.89 KB

Re-roll and fixed 2-5 from #5

Status: Needs review » Needs work

The last submitted patch, 6: 2353347-6.patch, failed testing.

zaporylie’s picture

zaporylie
he/him
Primary language Polish
CreditAttribution: zaporylie commented
FileSize
2353347-8.patch2.75 KB
interdiff-2353347-2-8.txt1.94 KB

I missed one string. Should be ok this time.

zaporylie’s picture

zaporylie
he/him
Primary language Polish
CreditAttribution: zaporylie commented
Status: Needs work » Needs review
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

I think I know why this uses UrlHelper::parse() rather than parse_url(), but shouldn't we document that very briefly?

Well, there has been no special reason, just general usage, as I assumed that there is a good reason why Drupal has introduced its own wrapper for it.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Btw. I am not convinced that this belongs into views_ui, given that the code is part of views itself.

chx’s picture

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

I saw this error myself. The fix is good, Wim's concerns are addressed and UrlHelper::parse supports internal urls vs parse_url doesn't, I doubt we need to add this to every place where we call it.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 557bd08 and pushed to 8.0.x. Thanks!

  • alexpott committed 557bd08 on 8.0.x 
    Issue #2353347 by zaporylie, dawehner: Fixed Random failure in...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Fixed » Needs work 
[10-Oct-2014 14:24:59 UTC] Uncaught PHP Exception InvalidArgumentException: "The URI "base://AKI@&hO@" is invalid. You must use a valid URI scheme. Use base:// for a path, e.g., to a Drupal file that needs the base path. Do not use this for internal paths controlled by Drupal." at /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Url.php line 207

Just had an issue fail with the above problem - we've not quite fixed this one :(

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
2353347-16.patch1.95 KB

Well yeah, the "@" is also a problem. I would vote for now, let's generate a safe string and add another custom test string to check for those cases.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito commented 
+++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
@@ -460,6 +461,10 @@ protected function validatePath($path) {
+      $errors[] = $this->t('Invalid path');

Why is it invalid? Could we hint what is wrong to the user here?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
2353347-18.patch2.03 KB
interdiff.txt645 bytes

Maybe link to the spec?

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito commented
Status: Needs review » Needs work

wow, maybe too much?
Can we explicitly say the reserved chars (2.2. Reserved Characters) in that spec?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

Also an error message that is Invalid path, see Spec is very odd - no full stop. Personally I feel The path '@path' is invalid. is consistent with our other messages - for example the site front page and MenuLinkContentForm use a similar error message.

YesCT’s picture

YesCT CreditAttribution: YesCT commented

had one here too
#2341341-41: Change public 'name' property access on languages to getName() and add back setName()
base://>j*>&:}A

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
2353347-22.patch2.01 KB
interdiff.txt633 bytes

Maybe just that.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito commented
Status: Needs review » Reviewed & tested by the community

OK for me.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Good catch.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 068bb1a on 8.0.x 
    Issue #2353347 follow-up by dawehner, alexpott: Fixed Random failure in...

Status: Fixed » Closed (fixed)

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