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
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.
Comments
Comment #1
Wim LeersLOL — nice find :)
Comment #2
michaelfavia CreditAttribution: michaelfavia commentedAs there is no way to unset the default #maxlength except override which requires a set value:
256 > characters needed by d.o > 128
Comment #4
amitgoyal CreditAttribution: amitgoyal commentedUpdated patch for D7. Line requires "," at the end.
Comment #6
amitgoyal CreditAttribution: amitgoyal commentedI 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.
Comment #8
michaelfavia CreditAttribution: michaelfavia commentedReroll
Comment #10
x3cion CreditAttribution: x3cion commentedI guess it automatically runs against 8.x-dev, since this Issue is set to 8.x-dev?
Comment #11
dcam CreditAttribution: dcam commentedUpdating 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.
Comment #12
dobe CreditAttribution: dobe commentedHello,
I am going to create a test for this issue. Good luck to me! lol.
-Jesse
Comment #13
jmarkel CreditAttribution: jmarkel commentedThanks! I'm reviewing this as is - I'll look for your test!
Comment #14
jmarkel CreditAttribution: jmarkel commentedScreen shots - before and after the patch
Comment #15
jmarkel CreditAttribution: jmarkel commentedThe D8 patch works as expected (see screenshots in #14). Code looks good. Waiting on the test before saying it's fully RTBC.
Comment #16
jmarkel CreditAttribution: jmarkel commentedApplied 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.
Comment #17
jmarkel CreditAttribution: jmarkel commentedupdated tasks in summary.
Comment #18
dobe CreditAttribution: dobe commentedWith a big thanks to @lauriii for helping me with this patch.
Includes the Automated Test
-Jesse
Comment #20
jmarkel CreditAttribution: jmarkel commentedNice!
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.
Comment #21
jmarkel CreditAttribution: jmarkel commentedIt would be great if you could also reroll the patch for D7 to include the test and get that out of the way :-)
Comment #22
dobe CreditAttribution: dobe commentedI 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.
Comment #23
dcam CreditAttribution: dcam commentedHi 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!
Comment #24
dobe CreditAttribution: dobe commentedThank you, I will get working on this.
-Jesse
Comment #25
dobe CreditAttribution: dobe commentedChanging back to me :D
Comment #26
dcam CreditAttribution: dcam commentedHi 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.
Comment #27
dobe CreditAttribution: dobe commentedI 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!
Comment #28
jmarkel CreditAttribution: jmarkel commentedAwesome, 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 :-)
Comment #29
dobe CreditAttribution: dobe commentedHello,
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
Comment #32
dobe CreditAttribution: dobe commentedWoops had a missing ')'. Must accidently backspaced the last edit I did.
Comment #33
dobe CreditAttribution: dobe commentedSubmit for testing.
Comment #35
dcam CreditAttribution: dcam commentedHi 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:
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.
This comment needs a period.
I don't think it's necessary to document the @var. At least, I've never seen that done in a Drupal inline comment.
This comment is also missing a period.
"maxlenth" is misspelled and it needs another period.
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!
Comment #36
dcam CreditAttribution: dcam commentedSorry, setting status.
Comment #37
dobe CreditAttribution: dobe commentedI will update the patch.
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.
Comment #38
dobe CreditAttribution: dobe commentedPer your request I updated the patch to facilitate all your suggestions.
-Jesse
Comment #39
dobe CreditAttribution: dobe commentedFound some other edits/mistakes.
Comment #40
dobe CreditAttribution: dobe commentedhmm....
Comment #44
dcam CreditAttribution: dcam commented#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!
Comment #45
dobe CreditAttribution: dobe commentedAttached is D7 version.
-Jesse
Comment #48
dcam CreditAttribution: dcam commented#40 is still RTBC.
Comment #49
dobe CreditAttribution: dobe commentedYeah guessing the D7 patch failed because this is a D8 thread. Once D8 get's Committed we can just rerun those.
-Jesse
Comment #50
catchWhile 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.
Comment #51
dcam CreditAttribution: dcam commentedIt sounds like these assertions need to be removed.
Comment #52
amitgoyal CreditAttribution: amitgoyal commentedFrom #50 and #51, it sounds we don't need testWidgetValidation().
Please review updated patch which removes this function.
Comment #53
catchThanks.
Comment #55
alexpottCommitted 418b722 and pushed to 8.x. Thanks!
Comment #56
dcam CreditAttribution: dcam commentedSince this issue was found by and affects D.o, I'm going to assume it needs to be backported to D7.
Comment #57
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedHere is patch for D7.
Comment #60
dcam CreditAttribution: dcam commented#57 is RTBC. It increases the maxlength of the allowed file extensions element to 256.
Comment #61
mpotter CreditAttribution: mpotter commentedHaving 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:
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.
Comment #64
dcam CreditAttribution: dcam commentedComment #67
dcam CreditAttribution: dcam commentedComment #70
dcam CreditAttribution: dcam commentedComment #73
dcam CreditAttribution: dcam commentedComment #74
dobe CreditAttribution: dobe commented@dcam I think we need to wait till the D8 gets commited before testing will pass so probably better to be patient.
Comment #75
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted 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.