Updated: Comment #20
Following issue #1215730: Terminology update: don't say "Resolution" when we mean "Dimensions", we should change the name of the function file_validate_image_resolution into file_validate_image_dimensions and update documentation accordingly.
Note: the original issue described in this issue is now handled by #2110499: Unify the call and interfaces of image toolkit operations, only the function name change remains.
Reroll patch containing only the remaining parts.
Error messages will change.
Function name change.
The function file_validate_image_resolution in file file.inc can need some better error handling. Looking at the code (this issue is not created as a result of an actual error that occurred), I found that:
- Errors in image_scale are ignored: image_scale can be very resource intensive (multiple copies (of various sizes) in memory at the same time) and is thus error prone and should be checked, though I have to admit that this normally leads to a WSOD and error handling thus may seem futile :(.
- Errors in image_save are ignored: saving a file and chmod'ing it is error prone and should be checked.
- Error message 'The image is too large; the maximum dimensions are %dimensions pixels.' is misleading and should indicate something like "the image is too large and could not be resized to its maximum dimensions" (see also #1079116: Inaccurate text: Images must be smaller than !max pixels.).
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | 1079154-13.patch | 10.91 KB | fietserwin |
| #8 | 1079154-8.patch | 10.83 KB | fietserwin |
| #6 | 1079154-6.patch | 10.62 KB | fietserwin |
| #6 | intediff.txt | 3.67 KB | fietserwin |
| #4 | 1079154.patch | 9.2 KB | fietserwin |
Comments
Comment #1
dddave commentedComment #2
fietserwinAFAIC, This issue does not need to be backported, so changed version without adding backport tag
Comment #3
claudiu.cristea@fietserwin, can you provide a patch?
Comment #4
fietserwinOK, there we go.
Remarks:
- Will need a reroll after #2066219: Decouple image type from image extension gets in.
- Following #1215730: Terminology update: don't say "Resolution" when we mean "Dimensions", I changed the name of the method to file_validate_image_dimensions().
- Improved documentation: a.o. incorrect param type for File, may need a follow up for other occurrences in this file, or we add it to this patch. Type hinting the image factory is to get auto-completion and error-checking by e.g. PhphStorm.
Comment #5
claudiu.cristea@fietserwin, Thank you for the patch. I see the tests passed but I don't see how to manually test this.
You should use the fully-qualified namespace of the class name. This mean also "\" prefix. Use the interface name instead of implementation. In this case it would be
\Drupal\file\Entity\FileInterface.Also
FileInterfaceshould be used for type hinting.Remove this line.
Are we sure that saving failed because the image is too large? I'm not. What about "Saving the resized version of the image failed."?
Same here. How do we know the reasons for resizing/scale failure? Wouldn't just be enough: "Failed to resize the image."?
Comment #6
fietserwin#5: Manually testing: hack scale and save to return FALSE and upload a large image?
#5.1, #5.2: Good catch, but the interface is located under \Drupal\file.
Question: should I change all these erroneous type info in other doc comments in this file as well?
#5.3: Is there an accepted way of specifying type information for services we request from Drupal;;service()? As the get method (is by its very nature) untyped, my (PhpStorm) IDE is full of warnings about unknown methods and I don't have auto-completion. This is a recipe for errors that can be easily caught by static type checking that modern IDE's are able to perform.
I think that adding this kind of documentation has added value for all developers that view or work on this code.
This point remains open for discussion/ideas.
#5.4, #5.5: I rephrased too "The image exceeds the maximum allowed dimensions and ...". The trigger for resizing (and saving that resized version) lie in the fact that the dimensions of the uploaded image exceed the maximum allowed dimensions. So, IMO, a plain "resizing failed" is too short. People are uploading not resizing.
Can a native English speaking person have a look at the messages as well?
The patch also contains (and the interdiff also shows) some white space/indentation corrections.
Comment #7
claudiu.cristeaIndeed, sorry! I would not change only if is in the proximity of touched lines (1-2 lines). That's because we want to keep the patch clean and readable.
There's no established rule for sorting the "use" statements but as a good practice a saw the next pattern:
So, wouldn't be nice if we move
Drupal\file\FileInterfaceafterFile? (btw: What is reporting PhpStorm? IsDrupal\file\Entity\Filestill used in that file?)Just using
\Drupal::service('image.factory')is OK. We are inside a procedural function and, anyway, we cannot inject that service. You did the right thing, but remove that comment -- there's no standard, no best practice to do that.I'm not an English native speaker but "Check first..." sounds better to me. But I have no idea what is the correct usage. Really!
Overall, looks good. Thank you!
Comment #8
fietserwin#7.1 I reordered all the use statements. The Drupal\file\Entity\File is still used because many other functions are still type hinting File instead of FileInterface... That will have to wait. I will create a follow-up issue.
#7.2 Changed into a normal comment line. I will create another issue to see what other do or think.
#7.3: Just removed the word first.
Comment #9
fietserwinComment #10
claudiu.cristeaRTBC. Thank you!
Comment #11
fietserwinBTW, follow-up: #2099691: File file.module: Change type hinting of File into FileInterface.
Comment #12
alexpottPatch no longer applies.
Comment #13
fietserwinComment #14
fietserwinRerolled.
Comment #15
claudiu.cristeaComment #16
linl commented#14: 1079154-13.patch queued for re-testing.
Comment #18
claudiu.cristeaNeeds reroll
Comment #19
fietserwinWill be handled by #2110499: Unify the call and interfaces of image toolkit operations that also touches this code.
Comment #20
fietserwinHmmm, this patch also contained some improvements regarding the use of resolution versus dimensions, based on issue #1215730: Terminology update: don't say "Resolution" when we mean "Dimensions". So changing goal of this issue.
Remaining tasks:
- function file_validate_image_resolution
- docs of file_validate_is_image
Comment #21
mondrake@fietserwin now a patch in #847098: Revise image field min/max settings help text form for clarity addresses this terminology issue - is this duplicated then?
Comment #35
smustgrave commentedThank you for sharing your idea for improving Drupal.
We are working to decide if this proposal meets the Criteria for evaluating proposed changes. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or there is no community support. Your thoughts on this will allow a decision to be made.
Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.
Thanks!
Comment #36
smustgrave commentedI think this can be closed out. If you see https://www.drupal.org/node/3363700
file_validate_image_resolution was deprecated for 'FileImageDimensions' => ['maxDimensions' => $max_dimensions]