Problem/Motivation

When creating a view with a page display, the page view path does not accept an integer between 1 and 99999 as path.
Higher values are working.

Proposed resolution

Remaining tasks

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because this is a regression from D7
Issue priority Normal because this is an isolated case.
Prioritized changes The main goal of this issue is to fix a bug.
Disruption None
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

StryKaizer’s picture

Issue summary: View changes
geertvd’s picture

Status: Active » Needs review
FileSize
1.5 KB
geertvd’s picture

Issue tags: +Needs tests

We'll probably need tests for this

dawehner’s picture

Nice bug. I'm curious whether we should allow any integer values. Just curious what is the usecase for that?

+++ b/core/modules/views_ui/src/ViewEditForm.php
@@ -431,13 +431,20 @@ public function getDisplayDetails($view, $display) {
+              // @todo Views should expect and store a leading /. See:
+              //   https://www.drupal.org/node/2423913
+              $url = Url::fromUserInput('/' . ltrim($uri, '/'));

Should we throw a validation error?

dawehner’s picture

Nice bug. I'm curious whether we should allow any integer values. Just curious what is the usecase for that?

+++ b/core/modules/views_ui/src/ViewEditForm.php
@@ -431,13 +431,20 @@ public function getDisplayDetails($view, $display) {
+              // @todo Views should expect and store a leading /. See:
+              //   https://www.drupal.org/node/2423913
+              $url = Url::fromUserInput('/' . ltrim($uri, '/'));

Should we throw a validation error?

StryKaizer’s picture

Tested patch in #2, works as expected.

Attached you can find a test for this issue, and a patch with test + fix from #2

The last submitted patch, 6: test_only-views_page_displays_do-2474471-6.patch, failed testing.

Anonymous’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests

This seems a sane approach to me, but I don't think dawehner's question in #4 (on the usecase) was answered?

Tests were added, but we also need a beta evaluation here.

Some nitpicking:

  1. +++ b/core/modules/views/src/Tests/Wizard/PagePathTest.php
    @@ -0,0 +1,40 @@
    + * Tests the ability of the views page path to create views with certain path structures.
    

    This should be a single line.

  2. +++ b/core/modules/views/src/Tests/Wizard/PagePathTest.php
    @@ -0,0 +1,40 @@
    +      $view1 = array();
    

    Why should this be $view1 if it is the only view?

pjbaert’s picture

Assigned: Unassigned » pjbaert
Status: Needs work » Needs review
FileSize
2.86 KB
1.77 KB

I processed the remarks @pjonckiere mentioned in #8.
I'll add a beta evaluation later today.

Status: Needs review » Needs work

The last submitted patch, 9: interdiff-views_page_displays_6-9.patch, failed testing.

pjbaert’s picture

Status: Needs work » Needs review
FileSize
1.77 KB

Well... I added an interdiff.patch file. Silly me.
I would still like a review of my latest patch.

pjbaert’s picture

Assigned: pjbaert » Unassigned
Issue summary: View changes

Added the beta evaluation.

Nice bug. I'm curious whether we should allow any integer values. Just curious what is the usecase for that?

I certainly think there is a usecase for this.
For example; we should be able to add the date/year value in our path.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

Agreed with #12. Also, like the beta evaluation now specifies, this is a regression (I tested with views 7.x-3.10).

The patch looks good now. IS looks a bit short but sufficient to me and we have a beta evaluation.

When committed, we should comment in #2423913: Leading slash in link fields and views has different UX that we added a todo in this patch.

dawehner’s picture

Sorry here are just a couple if nitpicks...

It would be great to maybe also expand the test coverage in \Drupal\views\Tests\Plugin\DisplayPageWebTest

  1. +++ b/core/modules/views/src/Tests/Wizard/PagePathTest.php
    @@ -0,0 +1,40 @@
    + * @file
    + * Definition of Drupal\views\Tests\Wizard\PagePathTest.
    

    Nitpick: Should be Contains \....

  2. +++ b/core/modules/views/src/Tests/Wizard/PagePathTest.php
    @@ -0,0 +1,40 @@
    +  function testPagePaths() {
    +
    

    let's make it public

  3. +++ b/core/modules/views/src/Tests/Wizard/PagePathTest.php
    @@ -0,0 +1,40 @@
    +    foreach($page_paths_to_test as $path){
    

    Let's use a space after 'foreach'

Anonymous’s picture

Status: Reviewed & tested by the community » Needs review
geertvd’s picture

Rewrote the test so we don't test the wizard but rather start from a yml file as suggested by dawehner via IRC.
This makes 1, 2 and 3 from #14 not applicable anymore since that file has been removed.

dawehner’s picture

Status: Needs review » Needs work

Looks fine for me!

+++ b/core/modules/views/src/Tests/Plugin/DisplayPageWebTest.php
@@ -134,4 +134,25 @@ public function testTitleOutput() {
+  public function assertPagePath($path) {

$path should be quickly documented :(

geertvd’s picture

ha views_page_displays_do-2474471-16-test.patch should be failing though.

geertvd’s picture

Fixed feedback in #17 and now we should have an actual failing test.

The last submitted patch, 19: views_page_displays_do-2474471-19-test.patch, failed testing.

geertvd’s picture

bump

koence’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
117.38 KB

List with integer between 1 and 99999 can be added.
Result of test added as attachment (screenshot).

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

webchick’s picture

Saving credit.

  • webchick committed aa991be on 8.0.x
    Issue #2474471 by geertvd, pjbaert, StryKaizer, koence, dawehner,...

Status: Fixed » Closed (fixed)

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