Problem/Motivation

Steps to reproduce:

  1. Enable views_ui
  2. Goto admin/structure/views/add
  3. Enter a view name longer than 128 characters
  4. See the following error messages:
    • Page title cannot be longer than 128 characters but is currently 200 characters long.
    • Path cannot be longer than 128 characters but is currently 200 characters long.
    • Link text cannot be longer than 128 characters but is currently 200 characters long.
    • Feed path cannot be longer than 128 characters but is currently 204 characters long.
    • Block title cannot be longer than 128 characters but is currently 200 characters long.

Proposed resolution

Add correct #maxlength to form elements created in \Drupal\views\Plugin\views\wizard\WizardPluginBase

  • Page title can be 255
  • Page path can be 254 because we have to account for the leading slash added
  • Link text can be 128
  • Feed path can be 254 because we have to account for the leading slash added
  • Block title can be 255

The prepolulate method in Drupal.viewsUi.FormFieldFiller needs to respect the maxlength of the field it is populating.

Remaining tasks

Write patch

User interface changes

Add max length to form fields.

API changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

Title: View names can be 200 characters long but this breaks » View names can be 255 characters long but this breaks stuff
Issue summary: View changes
Status: Active » Needs review
FileSize
4.7 KB
damiankloip’s picture

Looking pretty good.

  1. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php
    @@ -1124,7 +1124,7 @@ public function optionsSummary(&$categories, &$options) {
    +      'value' => views_ui_truncate($title, 32),
    

    Not the fault of this patch. We should move this somewhere more OO in a follow up. Isn't this a more general helper that we could just yoink out and put in out of our component classes?

  2. +++ b/core/modules/views_ui/js/views-admin.js
    @@ -132,7 +132,7 @@
    -      return from + this.suffix;
    

    This is handled below now?

alexpott’s picture

  1. Yep that is a helper function for the views_ui module
  2. Yep handled in the prepopulate method since the suffix length has to be subtracted from the maxlength to ensure that the prepopulated value is less than maxlength
damiankloip’s picture

Yep that is a helper function for the views_ui module

Yes, that is what I was saying :) We should consider moving it from Views UI module....

dawehner’s picture

+1 for the issue in general.

Berdir’s picture

1: 2220765.1.patch queued for re-testing.

xjm’s picture

Issue summary: View changes
FileSize
85.33 KB
alexpott’s picture

Issue tags: -Needs tests
FileSize
2.66 KB
7.51 KB

Now with added test

xjm’s picture

FileSize
55.36 KB
51.36 KB

Tested manually and confirmed the patch resolves the issue. Giving the view a very long label and saving now works fine, with no errors about view displays the user hasn't even added. Adding (e.g.) a page display also now works without error.

alexpott’s picture

FileSize
2.66 KB
7.51 KB
2.66 KB

Test only patch

xjm’s picture

Why isn't the bot picking up the test-only patch?

xjm’s picture

FileSize
2.66 KB
xjm’s picture

FileSize
2.63 KB

Oh, I see why maybe. There's some funky at the top of the patch.

alexpott’s picture

FileSize
2.81 KB

Hmmm... the I created using the interdiff had some fluff I removed it but obviously it's still not a valid patch... cut a new patch from my branch.

alexpott’s picture

Going mad the patches in #13 and #14 are both fine. And hopefully will both fail as expected.

jthorson’s picture

xjm: alexpott: This test is hosed in the test database ... I'll have to sort out why and manually clear it up.

jthorson’s picture

K ... should be unblocked.

alexpott’s picture

Status: Needs work » Needs review

So the patch #10 called "2220765.8.patch" is the one to review.

alexpott’s picture

xjm’s picture

Priority: Major » Normal
Status: Needs review » Reviewed & tested by the community

All good. I even read the JavaScript.

I don't really think this is major, though, so downgrading.

dawehner’s picture

The code looks good! (beside of the usual unimportant nitpicks).

xjm’s picture

Now I am wondering what @dawehner saw that I didn't. :)

xjm’s picture

FileSize
7.51 KB

Re-uploading the patch for clarity.

dawehner’s picture

  1. +++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/WizardTest.php
    @@ -0,0 +1,65 @@
    +/**
    + * Tests the wizard.
    + */
    +class WizardTest extends WizardTestBase {
    

    I really like to point to the actual tested class using @see

  2. +++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/WizardTest.php
    @@ -0,0 +1,65 @@
    +
    +  public static function getInfo() {
    

    MIssing @inheritdoc

  3. +++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/WizardTest.php
    @@ -0,0 +1,65 @@
    +    $this->drupalPostForm('admin/structure/views/add', $view, t('Save and edit'));
    ...
    +    $this->assertText('Machine-readable name cannot be longer than 128 characters but is currently 129 characters long.');
    +    $this->assertText('Path cannot be longer than 254 characters but is currently 255 characters long.');
    +    $this->assertText('Page title cannot be longer than 255 characters but is currently 256 characters long.');
    +    $this->assertText('View name cannot be longer than 255 characters but is currently 256 characters long.');
    +    $this->assertText('Feed path cannot be longer than 254 characters but is currently 255 characters long.');
    +    $this->assertText('Block title cannot be longer than 255 characters but is currently 256 characters long.');
    

    Is there a clear distinction between using t and not for checking stuff happened on the page?

xjm’s picture

We don't do @inheritdoc for getInfo().

For #3, our old rules would say to use t(). But since it doesn't actually matter in practice here, I'd be inclined to start leaving it out to get rid of unnecessary coupling unless we are actually testing translation. What do you think?

+1 for the first point though, though it needn't block commit.

alexpott’s picture

FileSize
559 bytes
7.69 KB

1. Fixed (to make @dawehner :) )
3. I agree that's why I wrote it like that - there is precedence in \Drupal\field_ui\Tests\ManageFieldsTest

Berdir’s picture

2. We do @inheritdoc for getInfo() and setUp() now. See https://drupal.org/node/325974 and #338403: Use {@inheritdoc} on all class methods (including tests). The precedence argument works both ways :)

dawehner’s picture

Thank you, I am fine with not using t(), but even in this new file we are already inconsistent.

alexpott’s picture

FileSize
472 bytes
7.72 KB

No need to not follow standards.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

That seems an unfortunate number of places to have to manually set that property, but I guess it can't really be helped.

However, shouldn't this actually be like 166 or whatever, pursuant to #2220757: Limit length of Config::$id to something <= 166 characters?

xjm’s picture

Status: Needs review » Reviewed & tested by the community

No, this is an end-user-facing label. Not a machine-name-ID-thing.

Edit: Or rather, various user-facing labels and other things like path aliases, none of which are config IDs. :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Oops, I thought I had already committed this, so it totally dropped off my radar. :( Sorry about that!

Committed and pushed to 8.x. Thanks!

  • Commit 7ec9480 on 8.x by webchick:
    Issue #2220765 by xjm, alexpott: View names can be 255 characters long...

Status: Fixed » Closed (fixed)

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