Problem/Motivation

In D8's core/modules/file/src/Plugin/Field/FieldType/FileItem.php and D7's modules/file/file.field.inc uses textfield's default '#maxlength' => 128. On Drupal.org, we want more than that, #1246118: Allow uploading of videos (screencasts) to d.o.

Before screenshot

Proposed resolution

Increase the file field widget allowed extensions setting input to 256.

After screenshot

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Add automated tests Yes Instructions
Manually test the patch Novice Instructions
Embed before and after screenshots in the issue summary Novice Instructions
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions

User interface changes

API changes

Original report by @drumm

In D8's core/modules/file/src/Plugin/Field/FieldType/FileItem.php and D7's modules/file/file.field.inc uses textfield's default '#maxlength' => 128. On Drupal.org, we want more than that, #1246118: Allow uploading of videos (screencasts) to d.o.

CommentFileSizeAuthor
#57 drupal7-increase_maxlength_of_extensions-2277281-57.patch639 byteser.pushpinderrana
#52 interdiff-2277281-40-52.txt1.91 KBamitgoyal
#52 increase_maxlength_of_extensions-2277281-52.patch793 bytesamitgoyal
#45 increase_maxlength_of_extensions-2277281-45-D7.patch2.69 KBdobe
#45 increase_maxlength_of_extensions-2277281-45-D7-test.patch2.07 KBdobe
#44 after.png5.91 KBdcam
#44 before.png5.94 KBdcam
#40 increase_maxlength_of_extensions-2277281-40.patch2.83 KBdobe
#40 increase_maxlength_of_extensions-2277281-40-test.patch2.08 KBdobe
#39 increase_maxlength_of_extensions-2277281-39-test.patch2.08 KBdobe
#38 increase_maxlength_of_extensions-2277281-38.patch2.91 KBdobe
#38 increase_maxlength_of_extensions-2277281-38-test.patch2.16 KBdobe
#32 increase_maxlength_of_extensions-2277281-30.patch2.94 KBdobe
#32 increase_maxlength_of_extensions-2277281-30-test.patch2.19 KBdobe
#29 increase_maxlength_of_extensions-2277281-29.patch2.95 KBdobe
#29 increase_maxlength_of_extensions-2277281-29-test.patch2.21 KBdobe
#18 increase_maxlength_of_extensions-2277281-18.patch2.43 KBdobe
#18 increase_maxlength_of_extensions-2277281-18-test.patch1.65 KBdobe
#14 Issue_2277281_After.png31.66 KBjmarkel
#14 Issue_2277281_Before.png31.14 KBjmarkel
#8 increase_maxlength_of_extensions-2277281-8-D7.patch639 bytesmichaelfavia
#6 increase_maxlength_of_extensions-2277281-6-D7.patch639 bytesamitgoyal
#4 interdiff-2277281-2-4.txt567 bytesamitgoyal
#4 _increase_maxlength_of_extensions-2277281-4-D7.patch639 bytesamitgoyal
#2 _increase_maxlength_of_extensions-2277281-2-D7.patch638 bytesmichaelfavia
#2 _increase_maxlength_of_extensions-2277281-2-D8.patch793 bytesmichaelfavia
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

LOL — nice find :)

michaelfavia’s picture

As there is no way to unset the default #maxlength except override which requires a set value:

256 > characters needed by d.o > 128

Status: Needs review » Needs work

The last submitted patch, 2: _increase_maxlength_of_extensions-2277281-2-D7.patch, failed testing.

amitgoyal’s picture

Status: Needs work » Needs review
FileSize
639 bytes
567 bytes

Updated patch for D7. Line requires "," at the end.

Status: Needs review » Needs work

The last submitted patch, 4: _increase_maxlength_of_extensions-2277281-4-D7.patch, failed testing.

amitgoyal’s picture

Status: Needs work » Needs review
FileSize
639 bytes

I can apply D7 patch correctly on my local system (ubuntu) from the Drupal 7 core root,

$ git apply -v patches/_increase_maxlength_of_extensions-2277281-4-D7.patch
Checking patch modules/file/file.field.inc...
Applied patch modules/file/file.field.inc cleanly.

Retrying with patch file name changes.

Status: Needs review » Needs work

The last submitted patch, 6: increase_maxlength_of_extensions-2277281-6-D7.patch, failed testing.

michaelfavia’s picture

Status: Needs work » Needs review
FileSize
639 bytes

Reroll

Status: Needs review » Needs work

The last submitted patch, 8: increase_maxlength_of_extensions-2277281-8-D7.patch, failed testing.

x3cion’s picture

I guess it automatically runs against 8.x-dev, since this Issue is set to 8.x-dev?

dcam’s picture

Issue summary: View changes
Issue tags: +Needs tests

Updating in preparation for DrupalCon Austin sprints. See http://www.hook42.com/blog/prepping-drupalcon-austin-sprints-sprint-lead....

I'm pretty sure this will need a test, but it should be simple to add.

@x3cion
Yes, Testbot is only going to test this against D8.

dobe’s picture

Assigned: Unassigned » dobe

Hello,

I am going to create a test for this issue. Good luck to me! lol.

-Jesse

jmarkel’s picture

Thanks! I'm reviewing this as is - I'll look for your test!

jmarkel’s picture

Screen shots - before and after the patch

jmarkel’s picture

The D8 patch works as expected (see screenshots in #14). Code looks good. Waiting on the test before saying it's fully RTBC.

jmarkel’s picture

Applied and tested the D7 patch too - works great and code (what little there is :-) ) is good.

Once a test has been implemented for both D7 and D8 these patches should be ready for RTBC.

jmarkel’s picture

Issue summary: View changes

updated tasks in summary.

dobe’s picture

Assigned: dobe » Unassigned
Status: Needs work » Needs review
FileSize
1.65 KB
2.43 KB

With a big thanks to @lauriii for helping me with this patch.

Includes the Automated Test

-Jesse

The last submitted patch, 18: increase_maxlength_of_extensions-2277281-18-test.patch, failed testing.

jmarkel’s picture

Nice!

I suggest, though, that instead of testing for greater than 128, that it test for the new 256 character limit. This makes it so the test is for what the design intention is, rather than embodying the issue being solved. So you might need several assertions instead of one, to cover the main and the edge cases.

jmarkel’s picture

It would be great if you could also reroll the patch for D7 to include the test and get that out of the way :-)

dobe’s picture

I am confused about what your trying to say. As it is the first test I have created. The patch that failed (increase_maxlength_of_extensions-2277281-18-test.patch) shows a test where the user tries to enter in > 128 characters. Which will currently fail in D8, which is what the patch is suppose to fix. The second patch which embodies both #2's patch and the test shows that with that patch applied, the test will pass. Which is what I thought we were going for here?

Please explain in more detail what your looking for. I am not sure how to show that Drupal 8 currently passes what is trying to be fixed. Even if I add more assertions. One of them will have to fail in the current version of Drupal 8.

dcam’s picture

Hi dobe! Thanks so much for the hard work you've done on this issue! Writing a test on your first day is an awesome accomplishment.

I noticed the same thing that jmarkel did, but I think I can put it in other terms for you. I was hoping to comment on this issue later when I have the D8 code in front of me, but since this has already been brought up, I want to do it now in case you're working on it right away.

The problem I noticed is that you're testing for the absence of a particular error message, which doesn't take into account the possibility of something completely unexpected happening now or in the future. What I think should be done is to remove the current assertion. Instead, test that the field is successfully configured with a length of 256 or less. Assert that you can find a system message saying it was done (or some other indicator of success on the page). Then I would test it again with a length greater than 256 characters and assert that you can find the error message. If you do that, we should be completely covered for testing this behavior, no matter what happens to the field. You can see something similar in the file field widget validation test. They test the success and fail cases for uploading files with and without an allowed file type.

Let me know if you need any help!

dobe’s picture

Thank you, I will get working on this.

-Jesse

dobe’s picture

Assigned: Unassigned » dobe

Changing back to me :D

dcam’s picture

Hi dobe, I'm glad to see you're still interested in working on this issue. Thanks for helping us all out!

There's one other thing that I noticed about your patch that I would like you to think about. This is why I was hoping to respond while I was in front of my IDE. A couple of years ago I was one of the people being mentored at a sprint. tim.plunkett was my mentor and one of the things he said stuck with me. He basically told us that most of the time if you add a brand-new test method then you're doing things wrong. You should always try to expand an existing test with new assertions. The reason why is because Simpletest sets up a new Drupal environment for every single test method (maybe it's every class, but I don't think so). Therefore, creating new methods adds significant overhead to testing.

That said, I've checked and I'm not sure there is any method which would be better to expand with your assertions. So all your work isn't for nothing. There is, however, a pre-existing test class which deals with testing the file field widget. You can find it in core/modules/file/src/Tests/FileFieldWidgetTest.php. I encourage you to check it out for yourself and see whether it would be better to move your test method into it, just to keep everything dealing with the widget in one place. I think you could just copy and paste your method into it.

In the spirit of keeping future devs from creating new methods, I have one last suggestion: change the name of your method to something more general, like testWidgetSettings(). There are other widget settings fields which don't appear to have any existing test coverage, at least not in the widget test class! Changing the method name will give people who may come along after you a hint that they should add to the method you created instead of making a new one.

dobe’s picture

I appreciate your time with this. Yes, tim.plunkett would be an amazing mentor. The classes I was looking at, which was a good handful of them just didn't have anything near appropriate to this issue (I knew I was missing something). Since I am pretty new to Drupal 8 I really was just trying to get something up that I would get this wonderful feedback and direction to. I will definitely take these suggestions to heart and put them to action! Your work with me has not gone in vain. I am very malleable.

Thank you again!

jmarkel’s picture

Awesome, dobe, and thanks for jumping in, dcam. I've been traveling back home so hadn't gotten back to this til now, having recuperated from a 6 AM flight after being out until midnight :-)

dobe’s picture

Hello,

I am back and here are the new renditions of the patches. Please let me know if I headed in the right direction. If I did I will get the D7 version done. If not I will work on what is suggested. Thanks for your guys help!

-Jesse

The last submitted patch, 29: increase_maxlength_of_extensions-2277281-29-test.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 29: increase_maxlength_of_extensions-2277281-29.patch, failed testing.

dobe’s picture

Woops had a missing ')'. Must accidently backspaced the last edit I did.

dobe’s picture

Status: Needs work » Needs review

Submit for testing.

The last submitted patch, 32: increase_maxlength_of_extensions-2277281-30-test.patch, failed testing.

dcam’s picture

Hi dobe, you've done some great work here making sure the field setting is thoroughly tested! You're testing it more ways than I thought you would, so that's great. It just means errors are more likely to be caught.

I have a few notes for you. The first one is that I expected all four of your assertions to be fail in the test-only patch, but instead only one is. I see that with this following line you're trying to test the setting validation for whatever its maxlength might be:

+++ b/core/modules/file/src/Tests/FileFieldWidgetTest.php
@@ -358,4 +358,41 @@ function testWidgetValidation() {
+    $maxlength = $file_extensions[0]['maxlength'];

I don't see any reason why we shouldn't go ahead and test it against the value that we expect it to be, 256. If you have a reason why we shouldn't, please feel free to explain.

Here are some additonal notes, just to make sure the patch meets the Drupal coding standards and documentation standards.

  1. +++ b/core/modules/file/src/Tests/FileFieldWidgetTest.php
    @@ -358,4 +358,41 @@ function testWidgetValidation() {
    +    // Tests for the file field widget settings
    

    This comment needs a period.

  2. +++ b/core/modules/file/src/Tests/FileFieldWidgetTest.php
    @@ -358,4 +358,41 @@ function testWidgetValidation() {
    +    // @var  $maxlength
    

    I don't think it's necessary to document the @var. At least, I've never seen that done in a Drupal inline comment.

  3. +++ b/core/modules/file/src/Tests/FileFieldWidgetTest.php
    @@ -358,4 +358,41 @@ function testWidgetValidation() {
    +    // Create the file field
    

    This comment is also missing a period.

  4. +++ b/core/modules/file/src/Tests/FileFieldWidgetTest.php
    @@ -358,4 +358,41 @@ function testWidgetValidation() {
    +    // The allowed file extensions should have the correct maxlenth
    

    "maxlenth" is misspelled and it needs another period.

  5. +++ b/core/modules/file/src/Tests/FileFieldWidgetTest.php
    @@ -358,4 +358,41 @@ function testWidgetValidation() {
    +      if($length <= $maxlength){
    +        $this->assertText(t("Saved $field_name configuration."), 'The allowed file extensions were successfully saved.');
    +      } else {
    +        $error_message = t('Allowed file extensions cannot be longer than %maxlength characters but is currently %length characters long.', array('%maxlength' => $maxlength, '%length' => $length));
    +        $this->assertRaw($error_message, 'Validation error when allowed file extensions are longer than maxlength.');
    +      }
    +    }
    

    A space is needed after "if" and before the opening bracket.

    The first assertion needs to use variable placeholders in the string, just like you're doing with the string in the second assertion.

    The else statement should be on its own line.

In my opinion, this patch is almost ready to be RTBC. Keep up the great work!

dcam’s picture

Status: Needs review » Needs work

Sorry, setting status.

dobe’s picture

I will update the patch.

I don't see any reason why we shouldn't go ahead and test it against the value that we expect it to be, 256. If you have a reason why we shouldn't, please feel free to explain.

In response: My thinking behind this was that if "something" were to break the maxlength value we would catch that no matter due to the first test, so if only one error pops for that case the developer that has failed his/her testing, they would be able to debug the issue a lot faster as it wouldn't show 3 errors for the 1 error (the other two being less descriptive about the actual cause). These other tests shouldn't (imo) care about what the maxlength is set to we are not testing that anymore (we already tested it).

So in one line: I feel that it would ease developer stress :D

If I have to change it to get this RTBC'd let me know.

dobe’s picture

Per your request I updated the patch to facilitate all your suggestions.

-Jesse

dobe’s picture

Found some other edits/mistakes.

dobe’s picture

The last submitted patch, 39: increase_maxlength_of_extensions-2277281-39-test.patch, failed testing.

The last submitted patch, 40: increase_maxlength_of_extensions-2277281-40-test.patch, failed testing.

The last submitted patch, 38: increase_maxlength_of_extensions-2277281-38-test.patch, failed testing.

dcam’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests
FileSize
5.94 KB
5.91 KB

#40 is RTBC. The patch correctly increases the maxlength of the file field widget allowed extensions input to 256. Screenshots are embedded in the issue summary.

@dobe
I wasn't sure what the best thing to do is either, so I gave your comments some more thought instead of responding last night.

The point to creating this test was simply to ensure that the maxlength isn't mistakenly reverted to a shorter length in the future. My initial suggestion that the length validation messages be tested was not well thought out. Your method of inspecting the field's maxlength directly is better. So in that sense, in #32 the only thing you're doing with the last three assertions is testing the messages produced by the form/field APIs. Hopefully they're already being tested by those APIs. As a result, your explanation in #37 is applicable to that situation. Simply having those assertions in this test potentially leads to confusing error log spam, such as in the case of something changing in those other APIs.

My thinking is that if we are to leave the three message-checking assertions in the test, then at least by checking it against the expected value there is some potential benefit in line with the point of adding this test. Devs should be able to see at a glance whether the value is too low or higher than expected, which could be useful when they can't view the verbose output here on D.o. So, I would still go with the solution in #40.

Good work, dobe!

dobe’s picture

The last submitted patch, 45: increase_maxlength_of_extensions-2277281-45-D7-test.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 45: increase_maxlength_of_extensions-2277281-45-D7.patch, failed testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community

#40 is still RTBC.

dobe’s picture

Yeah guessing the D7 patch failed because this is a D8 thread. Once D8 get's Committed we can just rerun those.

-Jesse

catch’s picture

While I appreciate the work on writing a test for this, I'm not sure we need explicit test coverage here - #maxlength should be tested generically elsewhere in the code base.

dcam’s picture

Status: Reviewed & tested by the community » Needs work

It sounds like these assertions need to be removed.

+++ b/core/modules/file/src/Tests/FileFieldWidgetTest.php
@@ -358,4 +358,39 @@ function testWidgetValidation() {
+
+    // Now we test to make sure the maxlength is respected.
+    foreach (array($maxlength-1, $maxlength, $maxlength+1) as $length) {
+      $edit = array(
+        'instance[settings][file_extensions]' => $this->randomName($length),
+      );
+      $this->drupalPostForm($settingsURL, $edit, t('Save settings'));
+      if ($length <= $maxlength) {
+        $this->assertRaw(t("Saved %field_name configuration.", array('%field_name' => $field_name)), 'The allowed file extensions were successfully saved.');
+      }
+      else {
+        $error_message = t('Allowed file extensions cannot be longer than %maxlength characters but is currently %length characters long.', array('%maxlength' => $maxlength, '%length' => $length));
+        $this->assertRaw($error_message, 'Validation error when allowed file extensions are longer than maxlength.');
+      }
+    }
+  }
amitgoyal’s picture

Status: Needs work » Needs review
FileSize
793 bytes
1.91 KB

From #50 and #51, it sounds we don't need testWidgetValidation().

Please review updated patch which removes this function.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Thanks.

  • alexpott committed 418b722 on 8.x
    Issue #2277281 by dobe, amitgoyal, michaelfavia | drumm: Sometimes you...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 418b722 and pushed to 8.x. Thanks!

dcam’s picture

Version: 8.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)
Issue tags: +Needs backport to D7

Since this issue was found by and affects D.o, I'm going to assume it needs to be backported to D7.

er.pushpinderrana’s picture

Status: Patch (to be ported) » Needs review
FileSize
639 bytes

Here is patch for D7.

Status: Needs review » Needs work

The last submitted patch, 57: drupal7-increase_maxlength_of_extensions-2277281-57.patch, failed testing.

Status: Needs work » Needs review
dcam’s picture

Status: Needs review » Reviewed & tested by the community

#57 is RTBC. It increases the maxlength of the allowed file extensions element to 256.

mpotter’s picture

Having trouble adding #57 to the drupal-org-core.make file in the Open Atrium distribution. Gives an error in the whitelist validation on drupal.org:

The patch 'http://www.drupal.org/files/issues/drupal7-increase_maxlength_of_extensi...' is not hosted on drupal.org [error]
The Drupal.org validation check failed -- see http://drupal.org/node/1432190 for more information. [error]

Would be nice to get this committed (guess it didn't make it for 7.30?) or added to the whitelist so people can use it.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 57: drupal7-increase_maxlength_of_extensions-2277281-57.patch, failed testing.

Status: Needs work » Needs review
dcam’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 57: drupal7-increase_maxlength_of_extensions-2277281-57.patch, failed testing.

Status: Needs work » Needs review
dcam’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 57: drupal7-increase_maxlength_of_extensions-2277281-57.patch, failed testing.

Status: Needs work » Needs review
dcam’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 57: drupal7-increase_maxlength_of_extensions-2277281-57.patch, failed testing.

Status: Needs work » Needs review
dcam’s picture

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

@dcam I think we need to wait till the D8 gets commited before testing will pass so probably better to be patient.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks! I gave @dcam commit credit on this too (even though Dreditor only automatically lists people who posted patches), due to some awesome mentoring and code reviews :)

The Drupal 8 patch was already committed - the test failures here were just due to the testbot being flaky.

  • David_Rothstein committed 2200030 on 7.x
    Issue #2277281 by dobe, amitgoyal, michaelfavia, er.pushpinderrana, dcam...

Status: Fixed » Closed (fixed)

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