Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Steps to reproduce:
- Enable views_ui
- Goto
admin/structure/views/add
- Enter a view name longer than 128 characters
- 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
Comment | File | Size | Author |
---|---|---|---|
#30 | 2220765.30.patch | 7.72 KB | alexpott |
#30 | 27-30-interdiff.txt | 472 bytes | alexpott |
#27 | 2220765.27.patch | 7.69 KB | alexpott |
#27 | 8-27-interdiff.txt | 559 bytes | alexpott |
#24 | 2220765.8.patch | 7.51 KB | xjm |
Comments
Comment #1
alexpottComment #2
damiankloip CreditAttribution: damiankloip commentedLooking pretty good.
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?
This is handled below now?
Comment #3
alexpottComment #4
damiankloip CreditAttribution: damiankloip commentedYes, that is what I was saying :) We should consider moving it from Views UI module....
Comment #5
dawehner+1 for the issue in general.
Comment #6
Berdir1: 2220765.1.patch queued for re-testing.
Comment #7
xjmComment #8
alexpottNow with added test
Comment #9
xjmTested 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.
Comment #10
alexpottTest only patch
Comment #11
xjmWhy isn't the bot picking up the test-only patch?
Comment #12
xjmComment #13
xjmOh, I see why maybe. There's some funky at the top of the patch.
Comment #14
alexpottHmmm... 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.
Comment #15
alexpottGoing mad the patches in #13 and #14 are both fine. And hopefully will both fail as expected.
Comment #16
jthorson CreditAttribution: jthorson commentedxjm: alexpott: This test is hosed in the test database ... I'll have to sort out why and manually clear it up.
Comment #17
jthorson CreditAttribution: jthorson commentedK ... should be unblocked.
Comment #19
alexpottSo the patch #10 called "2220765.8.patch" is the one to review.
Comment #20
alexpottComment #21
xjmAll good. I even read the JavaScript.
I don't really think this is major, though, so downgrading.
Comment #22
dawehnerThe code looks good! (beside of the usual unimportant nitpicks).
Comment #23
xjmNow I am wondering what @dawehner saw that I didn't. :)
Comment #24
xjmRe-uploading the patch for clarity.
Comment #25
dawehnerI really like to point to the actual tested class using @see
MIssing @inheritdoc
Is there a clear distinction between using t and not for checking stuff happened on the page?
Comment #26
xjmWe 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.
Comment #27
alexpott1. Fixed (to make @dawehner :) )
3. I agree that's why I wrote it like that - there is precedence in
\Drupal\field_ui\Tests\ManageFieldsTest
Comment #28
Berdir2. 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 :)
Comment #29
dawehnerThank you, I am fine with not using t(), but even in this new file we are already inconsistent.
Comment #30
alexpottNo need to not follow standards.
Comment #31
webchickThat 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?
Comment #32
xjmNo, 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. :)
Comment #33
webchickOops, I thought I had already committed this, so it totally dropped off my radar. :( Sorry about that!
Committed and pushed to 8.x. Thanks!