Problem/Motivation
Steps to reproduce (screencast):
$ touch zero.png
- Create a new node of type article and upload zero.png as the image.
- Save the node.
- The validation error below is seen and there is a watchdog error: "Unable to generate the derived image located at public://styles/thumbnail/public/field/image/zero.png"
Why this should be an RC target
Users often don't even realize that they are using invalid images. Having good error messages is important in allowing folks to know how to correct the problem. I don't know what a "correct primitive type" is, and I've been in this business for 16 years.
Proposed resolution
- introduce the 'file_validate_is_image' upload validator in ImageWidget
- in simpletest/files, add two files that are invalid image files for testing purposes, and add a validation test.
Remaining tasks
- review patch
User interface changes
Improved error message when attempting to upload an invalid image file
API changes
None
Comment | File | Size | Author |
---|---|---|---|
#125 | after.png | 68.12 KB | oakulm |
#125 | before.png | 62.54 KB | oakulm |
#121 | 2377747-121.patch | 4.56 KB | mondrake |
#121 | interdiff_117-121.txt | 994 bytes | mondrake |
#117 | 2377747-117.patch | 4.57 KB | mondrake |
Comments
Comment #1
cilefen CreditAttribution: cilefen commentedComment #2
mondrakeThis is borderline critical. IMO, we should prevent uploading of zero-byte (or more in general, invalid) image files in the first place. Allowing invalid image files in the image field generates all sorts of consequent errors because the image toolkit would not be able to process them.
The behaviour described is due to the fact that currently an invalid image file gets uploaded to file storage when clicking the 'Upload' button. The image file then is attempted to be styled through ImageStyle::createDerivative(), but since the image toolkit refuses to process it we get the watchdog error "Unable to generate the derived image located at public://styles/thumbnail/public/field/image/zero.png", and the 'broken image' icon close to the image file name.
At the moment, the error "This value should be of the of the correct primitive type" gets displayed only when the form is submitted, i.e. when the invalid file has already been uploaded to file storage. Besides, the error message is quite obscure, and the field highlighted for error is the "alternative text" one, which is misleading.
Comment #3
mondrakeA test only patch to prove the issue. Working on a patch.
Comment #5
mondrakeThe patch in #3 introduces, on purpose, invalid image files in the simpletest/files directory. As a result, we need to be more picky in the other tests to select valid image files for testing, as just getting the single file at the top of the array returned by drupalGetTestFiles('image') will not necessarily return a valid file (in fact, it is returning an invalid one). This patch is still test-only to fix other tests. The expected outcome is to get to 2 fails in ImageFieldValidateTest, i.e. the attempt to upload invalid images in the image field of an article.
Comment #7
mondrakeOne more test to fix.
Comment #9
mondrakeNeed to rerun #7 once #2376039: Undefined property ContainerAwareEventDispatcherTest::results in run-tests.sh is fixed.
Comment #14
mondrakePatch, doing what is the proposed resolution as updated in the issue summary.
do-not-test patch includes the changes made to non-test code, and equals the interdiff from #7
An open issue here is that we now have a duplicate error message shown to users (see screenshot). Need to discuss if fix here or in follow up.
Comment #16
mondrakeA few more test fixes.
Comment #17
mondrakeActually, I did not like all the
foreach()
loops in the tests, so I suggest to have a new methodWebTestBase::drupalGetTestImageFile()
to do all the work.Comment #18
mondrakeThere is already a separate #2346893: Duplicate AJAX wrapper around a file field issue for the duplicate error message reported in #14, so leaving that to there.
Comment #21
cilefen CreditAttribution: cilefen commentedComment #22
mondrakeRerolled, only patch context changes.
Comment #23
mondrakeComment #24
mondrakeComment #27
adci_contributor CreditAttribution: adci_contributor commentedTry to reroll
Comment #28
mondrakeSee #2345695-4: Users are able to upload 0-byte images
Comment #31
mondrakeNeeds reroll.
Comment #32
rpayanmPlease review.
Comment #34
mondrakeRerolled + fix to ImageFieldValidateTest
Comment #35
mgiffordWell the error message we have now "This value should be of the correct primitive type." is pretty hard to understand.
I do really prefer the error message with the patch "The specified file zero.png could not be uploaded. The image is invalid, or not supported. Allowed types: png jpg jpeg gif"
Comment #36
mgiffordComment #37
mgiffordRealized that the duplication above was caused by #2346893: Duplicate AJAX wrapper around a file field - it goes away when both patches are applied.
I looked over the code and it seems good to me. Would be good to have someone look over it who is more familiar with the image.module though.
Comment #41
mondrakeReroll
Comment #42
mondrakeTo reduce the scope/size of the patch I reverted the changes to GDToolkit as they fit better in the separate issue #2477381: GDToolkit::getSupportedExtensions returns incomplete list.
Comment #43
mgiffordThat patch still seems to work fine. Didn't produce a screenshot though as it has the same issues as the one I tested in #35 did.
Comment #44
mgiffordActually, I attached this patch above with the last one from 2346893 it worked great:
Comment #45
mondrakeChanged more tests to use
drupalGetTestImageFile()
instead ofdrupalGetTestFiles('image')
.Actually I begin to think we need a separate issue just to introduce
WebTestBase::drupalGetTestImageFile()
, and postpone this issue on that. That part in this patch is now by far larger than the fix of the original issue...Any thoughts/suggestions?
Comment #47
mondrakeOops
Comment #49
mondrakeRerolled, added for the new testing infra.
Comment #51
mondrakeFixed failing tests switching recently added
drupalGetTestFiles('image')
calls todrupalGetTestImageFile()
.Comment #52
mgiffordDoes this need the
<h3>Why this should be an RC target</h3>
info & RC phase evaluation table? Also there's the "rc target triage" tag.I'd like this in for the 8.0 release for sure.
Comment #53
mondrake@mgifford - I think it could help, at least it could get exposure, but given the extent of changes to test code I have some doubts...
Comment #54
mondrakeComment #55
mgiffordComment #56
cilefen CreditAttribution: cilefen commentedI was just like "why am I following this issue?". Oh, I opened it.
Comment #57
eiriksmLooking at this now...
Comment #58
eiriksmQuick reroll (so that it still applies with
git apply
)Also changed one line that wrapped a comment line a bit early (very nitpick, I know).
Other than that, tested the patch manually. Error message is much more helpful, although as pointed out earlier, you can still get the one about "wrong primitive type" if you for example disable javascript. That one is really confusing, but luckily not easy to trigger. But this is a good step anyway.
Comment #59
eiriksmUnassigning myself for clarity
Comment #61
eiriksmOops. Wonder how that happened. One new test file fell out. Same patch, only with the "image-zero-size.png" file added back.
Comment #63
eiriksmHm, this then.
Comment #64
mgiffordThis is way better. So what needs to be done to get this RTBC?
Comment #65
mondrakeRerolled after commit of #2485761: Support for transparent GIFs broken. Only patch context changes.
Comment #66
mondrakeChanged last test to use
drupalGetTestImageFile()
instead ofdrupalGetTestFiles('image')
. Also made a more appropriate selection of test image files inImageFieldDefaultImagesTest
to take 'normal' images rather than images specific for testing GD toolkit.Comment #67
eiriksmLgtm!
Comment #68
mondrakeHidden previous patches to avoid re-running of tests
Comment #69
catchIs this the only non-test change?
If so that makes the patch 8.1.x, but the test changes look like we might want to do those in 8.0.x too.
Are all the test changes really necessary to get the text change in? If not we should probably split the patch into the text change and the test improvements.
Leaving at 8.0.x for now - I'd probably split the text change into an 8.1.x issue and leave this for the test coverage.
Comment #70
mondrake#69:
this
and this
are the non-test code changes.
Well I do not know in detail. The entire point is that by introducing a zero size PNG file in the image test files, any existing test that just gets the first file returned by
drupalGetTestFiles('image')
is bound to failure :(See the trivial patch in #2512116-62: Ignore, testing issue to this point. 37 failures. Not a single line of code. Just a single file breaking the sequence. I think we need to get rid of this WTF...
We need that test file, at least if we want to test the case of uploading a zero-size image...
TBH I am more inclined to postpone this to 8.1, if nothing else because any contrib module with tests calling
drupalGetTestFiles('image')
is also likely to break tests right now if we do that in 8.0.x. Maybe there are not many around, but still. In 8.1 we might even consider 'disabling' the 'image' option fromdrupalGetTestFiles()
(throwing an exception?) and just go for getting named files viadrupalGetTestImageFile()
.Comment #71
mondrakeBTW this
are now probably false negatives since upload directory has a /year/month structure
Comment #72
catchOK let's move this to 8.1.x, leaving CNW for the points in #71.
Comment #73
mondrakeAddressing points in #71.
Comment #75
mondrakePatch in #73 is green
Comment #78
mondrakeNeeds reroll.
Added back RC target triage tag removed in #73. Let's see to get this in 8.1?
Comment #79
mondrakeRerolled, only patch context changes.
Comment #81
mondrakeIn the new test, changed from drupal_realpath to FileSystem::realpath. Did not touch instances in existing tests (out of scope here I'd say).
Comment #83
mondrakeComment #84
mgiffordLooking good to me.
Comment #86
mondrakeRerolled after commit of #2641588: Replace deprecated usage of entity_create('file') with a direct call to File::create().
Comment #87
mondrakeComment #88
yoroy CreditAttribution: yoroy commentedReview of the wording of the error messages based on the screenshot in #84:
Not really in scope of this patch, but we could do without the "specified" in the first error message "The specified file zero.png could not be uploaded."
"The image is invalid or not supported". This is of course *a lot* more specific than the "This value should be of the correct primitive type" nonsense :) Bu, at first look I did still wonder what it means to have an invalid image.
Our error messages often don't provide clear instructions on how to recover from the error. The how to reproduce in the issue summary works from a file named zero.png. The error message says "The image is invalid or not supported".
The second bit "Allowed types: jpg, png, gif" doesn't help in this scenario, because I *am* uploading a jpg. We want to be careful about not frustrating people even more with providing not really useful information.
I imagine the recovery strategy would probably be to have people try to open the image in an editor. If it doesn't open there either it should be clear that there's something wrong with the image. But that's too long a story to have up there in an error message.
Also, we're subtly mixing "file", "image", and "image type" in these three short sentences. Could we do something like:
Comment #90
mondrake@yoroy thanks for review.
1. Removed 'specified' from the first part of the message. That part of the message is managed by
file_save_upload
which is generically handling files upload, so I cannot find any easy way to replace 'file' with 'image'. So we end up withThe file zero.png could not be uploaded
.2. Changed the second part of the message as suggested.
Comment #91
mondrakeScreenshot:
Comment #92
yoroy CreditAttribution: yoroy commentedThanks! No worries about that "file" in the first sentence, I figured that might be the case.
Comment #94
mondrakeBroken by #2496867: Translatable image file is not working unless you also config the image field. Config can get lost anyway.. Puff, rerolling this is painful :(
Comment #95
mondrakeFound a funny bug in Drupal\image\Tests\ImageOnTranslatedEntityTest that was committed with #2496867: Translatable image file is not working unless you also config the image field. Config can get lost anyway..
Calls to
$this->getDrupalTestFiles('image')[...]
were all leading to the same file being picked (image-test.png), but from different directories and/or copies.This is because, in the test, the test file is uploaded to a field which stores the uploaded file in the same directory structure, and getDrupalTestFiles re-scans the public:// directory and its subdirectories every time it is called.
So calling
$this->getDrupalTestFiles('image')[0]
you get the FIRST of these files:the file is uploaded to
public://year-month/image-test.png
, so when you call$this->getDrupalTestFiles('image')[1]
, you will get the SECOND file fromwhich still is the SAME file that was picked before! And so on.
Comment #96
mondrakeThe fix now includes selecting named files for upload in \Drupal\image\Tests\ImageOnTranslatedEntityTest, and preventing scanning subdirectories of public::\ for image test files in WebTestBase::getDrupalTestFiles.
Comment #98
Truptti CreditAttribution: Truptti at Axelerant commentedVerified the patch '2377747-96.patch' in comment #96 and the patch is working as expected.Appropriate error message is displayed.
The file zero.png could not be uploaded.
The image file is invalid or the image type is not allowed. Allowed types: png, jpeg, gif
Steps performed to verify the patch are as follows:
1.Reproduced the issue on local D8 site
2.Applied patch 2377747-96.patch
3.Verified if the new error message is displayed.
Attaching snapshot for reference.
Comment #99
Truptti CreditAttribution: Truptti at Axelerant commentedComment #100
alexpottThis change looks out of scope and changing the string means that translators need to translate this again.
I guess that all of these changes are here because we add image-invalid.png. Imo this is is a very fragile change and one that is out-of-scope for the issue. We could simply create an invalid image in the test and have way less change in this patch and probably break less contrib tests as well.
Comment #101
mondrake#100:
1. Reverted.
2. I remain of the opinion that the current status is also very fragile as adding/removing test files results in test failures for those tests that expect a specific sequence of files (see for example #95, or #2644468-17: Multiple image upload breaks image dimensions). A method to get a test file by its filename would be pretty useful. But for the sake of getting this going I suppose that can be discussed elsewhere, so here I am reverting most of the changes.
Comment #102
mondrakeComment #104
xjmComment #105
joyceg CreditAttribution: joyceg commentedIt looks better now.
Tested successfully.
Comment #106
cilefen CreditAttribution: cilefen commented#2719317: Strange error when uploading an invalid file into a required file/image field
Comment #107
chetan2111 CreditAttribution: chetan2111 at Srijan | A Material+ Company commentedHi @mondrake,
I verified 2377747-101.patch on simplytest.me and it works. here are the following screen-shots.
Before applying patch screen-shot :
After applying patch screen-shot :
Comment #110
weri CreditAttribution: weri at Previon Plus AG commentedComment #111
oakulm CreditAttribution: oakulm as a volunteer commentedPatch 2377747-101.patch applies smoothly to latest 8.4.x-dev. Error message is much better now.
Comment #112
oakulm CreditAttribution: oakulm as a volunteer commentedComment #113
xjmNot really sure how this is major. It's a bug, certainly.
Comment #114
xjmThe new error message is far more helpful. Thanks everyone! Also thanks for the many screenshots confirming the new error message, although one would have been enough. :)
Since this includes a string change, moving to the minor branch. (Currently the branch setting and the old "minor version target" tag contradict each other, so I am resolving that by moving it to 8.4.x.) I do think the string change in the current patch is in scope because it is needed to correctly explain the new error that will now be covered by this error message.
We've had very difficult-to-debug datetime tests before because it's possible for the time to roll over during the test. This is a very fragile test that could introduce new random fails. Can we find a more reliable way of checking for the existence of the file?
Comment #115
mondrakeThis should do.
FWIW, I still think it's major because of #2, it's not just an UI improvement.
Comment #117
mondrakeRerolled after #2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase
Comment #120
mondrakeComment #121
mondrakeRe-rolled
Comment #123
mondrake#122 is a random test failure
Comment #124
oakulm CreditAttribution: oakulm as a volunteer commentedComment #125
oakulm CreditAttribution: oakulm as a volunteer commentedI tested the patch with latest version (8.5.x-dev) and the patch works as intended. Screens attached.
If there's nothing else to this should we move this issue forward?
Comment #126
alexpottNot the fault of this issue but at some point doesn't this need to be a constraint so the REST gets the same validation?
Comment #127
alexpottCrediting @xjm, @yoroy for their reviews.
Comment #128
alexpottCommitted 03e3d7a and pushed to 8.5.x. Thanks!
Someone needs to create the corresponding D7 issue and then mark this issue as fixed.
Comment #130
mondrakeI have reopened #2345695: Users are able to upload 0-byte images for the D7 backport - that issue was there already, but the starting point is different (in d7 0-byte files can be uploaded currently)
Comment #132
therobyouknow CreditAttribution: therobyouknow commentedWondering if this fix/these fixes have caused regression whereby webp is not supported.
Support for webp for example still not working in 8.5.4 it seems: https://www.drupal.org/project/drupal/issues/2313075#comment-12670991
Comment #133
april26 CreditAttribution: april26 commentedMy solution turned out to be the directory settings set under the content type, and not the title, image or GD. There was an invalid character in the upload directory name.