Closed (outdated)
Project:
Drupal core
Version:
11.x-dev
Component:
editor.module
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
4 Dec 2014 at 11:03 UTC
Updated:
8 Mar 2024 at 09:53 UTC
Jump to comment: Most recent
Comments
Comment #1
wim leersWow. Amazing find!
This used to work, but #657166: use × instead of x changed
xto×for the UI. I don't think we realized it'd affect validation. Clearly, tests are massively lacking here (there is zero test coverage forEditorImageDialog).Comment #2
aneek commented@Wim Leers,
By changing
$max_dimensions = $image_upload['dimensions']['max_width'] . '×' . $image_upload['dimensions']['max_height'];to$max_dimensions = $image_upload['dimensions']['max_width'] . 'x' . $image_upload['dimensions']['max_height'];may solve this issue. But if this section is missing a test case then it needs to be written in Editor module.Comment #3
wim leers#2: but that then breaks the work that was done in #657166: use × instead of x. That's not an option. We should just pass on the data in the expected format to
file_validate_image_resolution().Comment #4
aneek commented@Wim Leers, but then the function (
file_validate_image_resolution()) checks for image width and height exploded byxbut not with×.Isn't the issue #657166: use × instead of x only for front end UI? Sending
xin PHP code internally can be done right? if you agree then, inEditorImageDialog::buildForm()method @line 56,Instead of
(This uses the ×-mark instead of alphabetical x)
we could use,
Comment #5
wim leersYes, exactly :)
Comment #6
aneek commented@Wim Leers, okay so a quick patch can be made. But what are your thoughts on test cases? How the test cases would behave?
Comment #7
wim leersCurrently, there is zero test coverage for
EditorImageDialog. This issue doesn't need to add full test coverage, but it will have to start test coverage. It should be aWebTestBasetest. If it's a problem to get started with that, I can help out! :)Note: because a dialog is just a form rendered in a special way, you can just perform tests against the form itself, because it's a route like any other:
(Try it on your local D8: http://yoursite.local/editor/dialog/image/basic_html.)
Comment #8
aneek commented@Wim Leers, thanks.
Btw, this issue I've found while checking on this one. #2387205: Insert Image modal redirected to image upload form. Can you please have a look at it?
Comment #9
wim leersI just replied to that :)
Comment #10
aneek commentedOhh, I see it's duplicate. Never mind :-P
Comment #11
aneek commented@Wim Leers, thanks for the explanation. But help needed on how to validate the function
file_validate_image_resolution()?To validate the image dimension test, I think we have to set max_dimension in admin/structure/types/manage/article/fields/node.article.field_image page.
In Drupal's UI even if I save the max dimension to 400x400px and upload a much bigger image, in
EditorImageDialog::buildForm()the snippet$image_upload = $editor->getImageUploadSettings();doesn't give the array by which the code is conditioned.But the array that is returned is,
array ( 'status' => true, 'scheme' => 'public', 'directory' => 'inline-images', 'max_size' => '', 'max_dimensions' => array ( 'width' => 0, 'height' => 0, ), )As you see the array keys are different in both if condition and return array.
So for this the image is not re-sized to desired size.
I think this causes another bug. What are your thoughts?
Comment #12
wim leersYou're right; this is apparently broken. The root cause for that: we don't have any tests for
EditorImageDialogat all. That's my fault. I opened #2395377: Write integration test for EditorImageDialog to rectify that. Let's postpone this issue on that, because it'll be difficult to start test coverage from zero for fixing this exact problem.Apologies for having wasted some of your time, but I very much appreciate you taking this on!
Comment #13
aneek commented@Wim Leers, thanks for creating the test case issue. It was a nice learning experience in the image core to find the bug. So, no worries you haven't wasted my time :-)
Can you tell me one thing, will this bug with invalid array keys in image validation can be raised as a different issue or its been taken care of?
Thanks!
Comment #14
manningpete commentedRemoving the novice tag.
Why: because issue is postponed. People new to the issue queue might search for just the Novice tag and not know to filter to active issues. If/when issue is updated, consider re-adding the novice tag.
Novice tag documentation: https://www.drupal.org/core-mentoring/novice-tasks
Comment #26
quietone commentedThis issue was discussed at a Bug Smash Initiative group triage meeting. This is still valid, update IS a bit.
Comment #29
wim leers#3306584: [11.x] Remove EditorLinkDialog, EditorImageDialog and EditorMediaDialog in Drupal 11 landed, this is now obsolete.