Add test coverage for the editor's max dimension setting, and fix its message when one dimension is unrestricted

+++ b/core/modules/editor/editor.admin.inc
@@ -126,3 +127,15 @@ function editor_image_upload_settings_form(Editor $editor) {
+  if (!empty($element['width']['#value']) || !empty($element['height']['#value'])) {

Is this check really needed? A nesting of 3 levels just seems to be too much.

Hey Matt_five, the patch does fix the issue.

Can you make sure the code follows drupal standards, for example here the function does not have any comment block.

}

function editor_image_upload_settings_validate_dimension($element, &$form_state) {
  if (!empty($element['width']['#value']) || !empty($element['height']['#value'])) {

@Matt_five You should read the Coding Standards set by the Drupal community as the patch does not comply with them. In the meantime you could unassign the issue so somebody else could help move it forward.

Test only patch. Should fail

Fixed some coding standards issues and changed some checks of the test.

Looking great! :) This is definitely the right direction.

My remarks are mostly nitpicks, and organization/repetitiveness of the test. Should be easy to fix :)

Thanks so much for pushing this forward!

1. +++ b/core/modules/editor/editor.admin.inc
   @@ -126,3 +128,20 @@ function editor_image_upload_settings_form(Editor $editor) {
   + * #element_validate handler for the "max_dimensions" element in
   + * editor_image_upload_settings_form().
   

   Nit: Needs to be reduced to a single line. Look elsewhere in core for inspiration on how to do that.

2. +++ b/core/modules/editor/editor.admin.inc
   @@ -126,3 +128,20 @@ function editor_image_upload_settings_form(Editor $editor) {
   +  if (!empty($element['width']['#value']) || !empty($element['height']['#value'])) {
   ...
   +      if (!$element[$dimension]['#value']) {
   

   This Should use $form_state->getValue('width') etc.

3. +++ b/core/modules/editor/editor.admin.inc
   @@ -126,3 +128,20 @@ function editor_image_upload_settings_form(Editor $editor) {
   +        $form_state->setError($element[$dimension], t('Both a height and width value must be specified in the @name field.', array('@name' => $element['#title'])));
   

   Nit: s/@name/%name/

4. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,143 @@
   +  protected $profile = 'standard';
   

   This makes this test very very very very slow. This is fine for a first iteration of this test though, because it means less work for you :)

   But instead of doing this, either import the basic_html text format + text editor directly, or duplicate them.

5. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,143 @@
   +    $this->adminUser = $this->drupalCreateUser(array('administer filters'));
   

   s/array()/[]/, this is a new class, so let's use the short notation everywhere in this class.

6. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,143 @@
   +    $editor = entity_load('editor', 'basic_html');
   ...
   +    $editor = entity_load('editor', 'basic_html');
   

   Nit: Editor::load()

7. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,143 @@
   +    $this->assertUrl('admin/config/content/formats/manage/basic_html');
   

   Why this assert?

8. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,143 @@
   +    $width = '';
   +    $height = '400';
   +    $edit = array();
   +    $edit['editor[settings][plugins][drupalimage][image_upload][max_dimensions][width]'] = $width;
   +    $edit['editor[settings][plugins][drupalimage][image_upload][max_dimensions][height]'] = $height;
   +    $this->drupalPostForm(NULL, $edit, t('Save configuration'));
   +    $this->assertRaw(t('Both a height and width value must be specified in the @name field.', array('@name' => 'Maximum dimensions')));
   

   This can be split off into a helper method, this happens multiple times.

9. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,143 @@
   +    $editor = entity_load('editor', 'basic_html');
   +    $image_upload_settings = $editor->getImageUploadSettings();
   +    $this->assertEqual($image_upload_settings['max_dimensions']['width'], '', 'Max width dimension setting has not changed.');
   +    $this->assertEqual($image_upload_settings['max_dimensions']['height'], '', 'Max height dimension setting has not changed.');
   

   This can also be split off to a helper method.

@Matt_five: Glad I could help!

@Wim Leers: Thank you for the review! Not sure how to change that since I initially got inspired from \Drupal\ckeditor\Plugin\CKEditorPlugin\DrupalImage::validateImageUploadSettings() and \Drupal\ckeditor\Plugin\CKEditorPlugin\StylesCombo::validateStylesValue(). Also I wasn't able to do 2 for some reason. Could you please advice on that?

Interdiff review:

1. +++ b/core/modules/editor/editor.admin.inc
   @@ -139,7 +139,7 @@ function editor_image_upload_settings_validate_dimension(array $element, FormSta
      if (!empty($element['width']['#value']) || !empty($element['height']['#value'])) {
   

   These should still use $form_state->getValue().

2. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -17,13 +19,11 @@
   -  protected $profile = 'standard';
   +  public static $modules = ['editor', 'ckeditor'];
   

   Much better! I bet that was ten times faster to run :)

3. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -56,88 +71,68 @@ public function testEditorSettingsValidation() {
   +    $this->validateDimensions();
   ...
   +  protected function validateDimensions($width = '', $height = '') {
   

   This is confusing. If we'd pass the empty strings explicitly, it wouldn't be confusing.

   Same for setDimensions().

Overall patch review:

1. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,138 @@
   +    $this->format = FilterFormat::create([
   ...
   +    $editor = Editor::create([
   

   Not $this->format or $this->editor, because you don't use them later, and they're not class properties.

   Just $format and $editor.

   And in fact, not even that. Just FilterFormat::create([…])->save(); :)

2. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,138 @@
   +    // Go to the basic_html editor settings form.
   ...
   +    // Check for the editor "max_dimensions" width and height values.
   ...
   +    // Go to the basic_html editor settings form.
   ...
   +    // Check for the editor "max_dimensions" width and height values.
   ...
   +    // Go to the basic_html editor settings form.
   ...
   +    // Check for the editor "max_dimensions" width and height values.
   ...
   +    // Go to the basic_html editor settings form.
   ...
   +    // Check for the editor "max_dimensions" width and height values.
   

   Let's remove all these comments, none of them add anything meaningful, they're only distracting.

3. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,138 @@
   +    $this->assertResponse(200);
   ...
   +    $this->assertResponse(200);
   ...
   +    $this->assertResponse(200);
   ...
   +    $this->assertResponse(200);
   

   We're not testing 200 vs 400 here. We're always asserting 200. That's not very meaningful. If any response turns out to not be a 200, then all the other assertions will fail anyway. So, let's just remove these assertions and keep it simple, and focus on testing the things that really matter.

4. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,138 @@
   +    // Set width but don't set height.
   ...
   +    // Set height but don't set width.
   ...
   +    // Set both height and width.
   ...
   +    // Set both height and width to be empty.
   

   Once you've done the previous point, these 4 comments become the *only* comments. That's not by accident: these are the 4 cases you're testing. It's worth explicitly describing those. The rest is just details.

Hopefully this is should almost there :)

Yes! So much better :) Very, very close now. Next one will probably be RTBC.

1. +++ b/core/modules/editor/editor.admin.inc
   @@ -136,9 +136,15 @@ function editor_image_upload_settings_form(Editor $editor) {
   +  $width = &$form_state->getValue(array('editor', 'settings', 'plugins', 'drupalimage', 'image_upload', 'max_dimensions', 'width'));
   +  $height = &$form_state->getValue(array('editor', 'settings', 'plugins', 'drupalimage', 'image_upload', 'max_dimensions', 'height'));
   +  $dimensions = array(
   +    'width' => $width,
   +    'height' => $height,
   +  );
   +  if (!empty($width) || !empty($height)) {
   

   Why the reference?

   And you can simplify this to just:

   $dimensions = $form_state->getValue(array('editor', 'settings', 'plugins', 'drupalimage', 'image_upload', 'max_dimensions']);
   if (!empty($dimensions['width']) || !empty($dimensions['height']) {
   

2. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,124 @@
   +    $width = '300';
   +    $height = '';
   +    $this->setDimensions($width, $height);
   ...
   +    $this->validateDimensions('', '');
   

   Why not do $this->setDimensions('300', '') directly?

   The expected saved dimensions will not match this anyway, so these variables are useless.

3. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,124 @@
   +  protected function setDimensions($width = '', $height = '') {
   

   Nit: setMaxDimensions()

4. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,124 @@
   +    $edit = array();
   

   s/array()/[]/

5. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,124 @@
   +  protected function validateDimensions($width = '', $height = '') {
   

   This is a helper that combines a bunch of asserts. That should then also have the assert* prefix.

   I'd call this assertSavedMaxDimensions()

6. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,124 @@
   +    $this->assertEqual($image_upload_settings['max_dimensions']['width'], $width, 'Max width dimension setting equals expected value.');
   +    $this->assertEqual($image_upload_settings['max_dimensions']['height'], $height, 'Max height dimension setting equals expected value.');
   

   Omit these messages.

   The message will always be displayed, which means you won't actually see what the mismatch is. If you omit the message, you'll get things like "Expected '300' equals '300'." — i.e. useful output.

Thank you for the review! Let see :)

I wanted to RTBC, but then remembered #20.1 and #21. So, providing the answer you were looking for back then, sorry for forgetting about that in #24.

1. +++ b/core/modules/editor/editor.admin.inc
   @@ -126,3 +128,21 @@ function editor_image_upload_settings_form(Editor $editor) {
   +/**
   + * #element_validate handler for the "max_dimensions" element in
   + * editor_image_upload_settings_form().
   + *
   + * Validates that both the height and width dimensions have been provided.
   + */
   

   Suggested alternative:

   /**
    * #element_validate handler for the "max_dimensions" element.
    *
    * @see editor_image_upload_settings_form()
    *
    * @param array $element
    *   The form element.
    * @param \Drupal\Core\Form\FormStateInterface $form_state
    *   The form state.
    */
   

2. +++ b/core/modules/editor/editor.admin.inc
   @@ -126,3 +128,21 @@ function editor_image_upload_settings_form(Editor $editor) {
   +  $dimensions = $form_state->getValue(array('editor', 'settings', 'plugins', 'drupalimage', 'image_upload', 'max_dimensions'));
   ...
   +        $form_state->setError($element[$dimension], t('Both a height and width value must be specified in the %name field.', array('%name' => $element['#title'])));
   

   Nit: s/array()/[]/

3. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,120 @@
   +    // Case 1: Set width but don't set height and submit the form which should
   +    // return a validation error while the editor "max_dimensions" width and
   +    // height should be empty.
   ...
   +    // Case 2: Set height but don't set width and submit the form which should
   +    // return a validation error while the editor "max_dimensions" width and
   +    // height should be empty.
   ...
   +    // Case 3: Set both height and width and submit the form which should return
   +    // a success message while the editor "max_dimensions" width and height
   +    // should equal the ones submitted.
   ...
   +    // Case 4: Set both height and width as empty values and submit the form
   +    // which should return a success message while the editor "max_dimensions"
   +    // width and height should be empty.
   

   Thinking on this again, I think the long-form explanations here are a distraction. They describe the entire behavior that is asserted in the couple of lines that follow.

   I'd omit the and submit the form … portions of all of these. Then it's clear what the 4 cases are: they describe the 4 cases you test, not all the detailed consequences.

4. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,120 @@
   +    $editor = Editor::load('basic_html');
   +    $image_upload_settings = $editor->getImageUploadSettings();
   

   Nit: these two lines can be combined.

Once those are addressed, I think this patch looks absolutely perfect, and I'll be very confident that if I RTBC it, it will also actually be committed by a core committer :)

No worries :D Thank you for reviewing!

Perfect! :) Funny how the patch size decreased from 8 KB in #17 to <5.5 KB in #27!

Thanks so much!

1. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,111 @@
   +    $this->drupalGet('admin/config/content/formats/manage/basic_html');
   ...
   +    $this->drupalGet('admin/config/content/formats/manage/basic_html');
   ...
   +    $this->drupalGet('admin/config/content/formats/manage/basic_html');
   ...
   +    $this->drupalGet('admin/config/content/formats/manage/basic_html');
   ...
   +    $this->drupalPostForm(NULL, $edit, t('Save configuration'));
   

   I think the drupalPostForm() should have the url and the setMaxDimensions() should take an argument of the filter format ID. This way we have less code and setMaxDimensions() has less undeclared dependencies - in order to use it atm you need to do a drupalGet first...

2. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,111 @@
   +  protected function setMaxDimensions($width = '', $height = '') {
   

   Missing a docblock

3. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,111 @@
   +    $edit = [];
   +    $edit['editor[settings][plugins][drupalimage][image_upload][max_dimensions][width]'] = $width;
   +    $edit['editor[settings][plugins][drupalimage][image_upload][max_dimensions][height]'] = $height;
   

   $edit = [
     'editor[settings][plugins][drupalimage][image_upload][max_dimensions][width]' => $width,
     'editor[settings][plugins][drupalimage][image_upload][max_dimensions][height]' => $height,
   ];
   

4. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,111 @@
   +  protected function assertSavedMaxDimensions($width = '', $height = '') {
   

   Asserts are supposed to return a bool which is TRUE on success or FALSE. This method is also missing a doc block.

5. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,111 @@
   +    $this->assertEqual($width, $image_upload_settings['max_dimensions']['width']);
   +    $this->assertEqual($height, $image_upload_settings['max_dimensions']['height']);
   

   There is no meaningful message here.

1. Or instead just move this $this->drupalGet() call into setMaxDimensions(). No need to make the text format ID a parameter yet at this point.
2. I don't get what the point is to document protected helpers on test classes. It's a waste of time IMO.
3. Well that's a supernitpick, but sure. I didn't know this was the rule? We do it like the current patch just about everywhere.
4. Huh? That makes no sense IMO. Counter-examples: \Drupal\simpletest\WebTestBase::assertUrl(), \Drupal\simpletest\AssertContentTrait::assertTitle(). What matters is that you make an assertion, but that doesn't mean it's boolean. It can be a comparison. I'm very very confused by this statement.
5. Yes, because that actually provides meaningful output. You're the one who taught me that! :) Better to have output like Expected 55 equals 55. or Expected 55 does not equal 345 than Expected width found being a test or fail. The former is much more helpful.

Regarding point 4:

+++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
@@ -0,0 +1,111 @@
+    $this->assertEqual($width, $image_upload_settings['max_dimensions']['width']);
+    $this->assertEqual($height, $image_upload_settings['max_dimensions']['height']);

After a lot of comparing with existing assert*() implementations, I now see what Alex meant.

We should assign the result of these two statements to variables (say $a and $b), and then return the result: return $a && $b;.

But I've never realized that until now. I've done it wrong in every test I've ever written. And along with that, 99% of us.

Some of my violations:

• \Drupal\ckeditor\Tests\CKEditorTest::assertCKEditorLanguage()
• \Drupal\big_pipe\Tests\BigPipeTest::assertBigPipeResponseHeadersPresent()

Some of our collective violations:

• \Drupal\node\Tests\AssertButtonsTrait::assertButtons
• \Drupal\comment\Tests\CommentPagerTest::assertCommentOrder()
• \Drupal\config\Tests\SchemaCheckTestTrait::assertConfigSchema()

Regarding point 2: I discussed that with Alex Pott on IRC, he pointed out that this is a requirement of the current coding standard. Which is totally fair. I posted #2108785-22: Remove the requirement for doxygen for test methods to hopefully make that coding standard more sensible.

So, I agree with points 1, 2 and 3. Point 4 is IMO very questionable (but easy enough to comply with per #31) and point 5 I still disagree with (though of course I agree having a nicely formatted message that says "Expected width of 55 does not equal the expected width of 60" or "Expected width of 55 equals the expected width of 60" is much nicer, but we don't do that anywhere). Overall I don't understand this extreme nitpicking of tests, I'm very surprised by it.

I'd agree with point 4 (either making the assert method follow the normal pattern or renaming it, and followups to look at the other ones that don't follow the pattern). I can see that point 5 could be interpreted either way. assertTrue() and assertFalse() have useless assertion messages by default, so it is almost always better to override them (in contrast with like assertText() where the default assertion message is almost always better than what someone comes up with to replace it). assertEqual() is in between. For the best test debugging the assertion would both show the values that were equal (or not) and what they meant, exactly like the proposed one in #32.

This also reminded me of #1601146: Allow custom assertion messages using predefined placeholders again.

Did the changes according to #29 and #32.

@alexpott: If we want setMaxDimensions() to take format as an argument, maybe assertSavedMaxDimensions() should do that too. I would do what #30-1 suggested but didn't because of:

Wim Leers: So, I agree with points 1, 2 and 3.

#33: Thanks so much for linking to #1601146: Allow custom assertion messages using predefined placeholders, I didn't know about that at all!

1. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -67,45 +67,71 @@ protected function setUp() {
   +   * Sets the maximum width and height of the Editor image settings and submits
   +   * the configuration form of a given text editor format.
   ...
   +   * Asserts whether the width and height of the uploaded image equal the ones
   +   * expected by the Editor image settings.
   

   Violates 80 cols rule.

2. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -67,45 +67,71 @@ protected function setUp() {
   +   * @return void
   

   We don't ever write @return void. If nothing is returned, omit @return.

3. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -67,45 +67,71 @@ protected function setUp() {
   +    $same_width = $this->assertEqual($width, $expected['width'], 'Expected width of "' . $width . '" equals the expected width of "' . $expected['width'] . '"');
   

   You wrote "Expected width" twice. The first one is not the expected width, it's the actual width.

4. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,137 @@
   +   *   The text editor format of the form being submitted.
   

   This is not a text editor format, but just a text format.

5. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,137 @@
   +   *   The maximum width for the Editor image settings.
   ...
   +   *   The maximum height for the Editor image settings.
   

   s/Editor image/associated text editor's image upload/

6. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,137 @@
   +   *   The width of the uploaded image.
   ...
   +   *   The height of the uploaded image.
   

   s/The width/The expected width
   s/The height/The expected height/

1. I am not sure how this violates to 80 cols rule.
2. Here are some wrong examples of this practice:

\Drupal\Core\StreamWrapper\PhpStreamWrapperInterface::stream_close()
\Drupal\field\Tests\FieldUnitTestBase::entityValidateAndSave()
\Drupal\user\UserDataInterface::set()
\Drupal\user\UserDataInterface::delete()

Should I open a new issue to fix these?

Never mind about 1 I think I understand now.

AFAICT this addressed all of the feedback in #29 and #35.

Just a few more minor things and then this is ready!

1. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,134 @@
   +   * Sets the maximum dimensions and saves the configuration.
   

   Sets the maximum image dimensions using the UI.

2. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,134 @@
   +   *   The text format of the form being submitted.
   

   The text format whose associated text editor will be updated.

3. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,134 @@
   +  protected function setMaxDimensions($format = '', $width = '', $height = '') {
   

   s/$format/$format_id/

   More importantly: remove all the default argument values.

4. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,134 @@
   +    $this->drupalPostForm('admin/config/content/formats/manage/' . $format, $edit, t('Save configuration'));
   

   Since we're getting the format ID, we might as well use routes instead of using string concatenation.

   Let's do $this->drupalPostForm(Url::fromRoute('entity.filter_format.edit_form', ['filter_format' => $format_id]), …

5. +++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
   @@ -0,0 +1,134 @@
   +  protected function assertSavedMaxDimensions($width = '', $height = '') {
   

   Remove all the default argument values.

Applied changes and rerolled for 8.1.x

Thanks!

The French translation file could not be downloaded. Choose a different language or select English and translate your website later. in install_display_requirements() (line 2162 of /var/www/html/core/includes/install.core.inc). install_display_requirements(Array, Array) (Line: 1328)

Looks like l.d.o was down.

Yeah I don't think we are testing remote l.d.o download anywhere in the tests, but either way it is not a problem of this patch.

I've considered the upgrade path. I'm not sure there is anything we can do. We could warn the user that they need to fix them I guess. What happens if only one value is completed?

+++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
@@ -0,0 +1,135 @@
+/**
+ * @file
+ * Contains \Drupal\editor\Tests\EditorSettingsValidationTest.
+ */
+

@file blocks are now against the coding standard for autoloaded files. Can be fixed on commit if the answer to the upgrade question is that we don't need one.

No upgrade path is necessary. I agree there's nothing we can do.

Did some manual testing.

1. The problem

The configuration for the text editor is used in EditorImageDialog and is passed to file_validate_image_resolution(), which is the validation callback ultimately responsible.

Its expectation:

 * @param string|int $maximum_dimensions
 *   (optional) A string in the form WIDTHxHEIGHT; for example, '640x480' or
 *   '85x85'. If an image toolkit is installed, the image will be resized down
 *   to these dimensions. A value of zero (the default) indicates no restriction
 *   on size, so no resizing will be attempted.

It then uses this logic:

    if ($maximum_dimensions) {
      // Check that it is smaller than the given dimensions.
      list($width, $height) = explode('x', $maximum_dimensions);
      if ($image->getWidth() > $width || $image->getHeight() > $height) {

2. When walking through it with a debugger using manual testing, what happens?

• When you specify only a width, EditorImageDialog constructs the string 300x, which file_validate_image_resolution() parses into $width=300, $height="". It then does $image->getHeight() > $height, which always evaluates to TRUE, so it will always complain.
• When you specify only a height, EditorImageDialog constructs the string x300, which file_validate_image_resolution() parses into $width="", $height=300. It then does $image->getWidth() > $width, which always evaluates to TRUE, so it will always complain.
• When you specify both, file_validate_image_resolution() works as expected/intended.
• When you specify neither, EditorImageDialog passes $max_dimensions = 0 to file_validate_image_resolution(), which is also correct, and behaves as expected/intended.

Note: #2628656: Max dimensions do not apply to Editor Inline image is the other issue in this area that must land for this functionality to work correctly: EditorImageDialog both accesses the wrong keys and constructs that 500x500 (width x height) string incorrectly… Yes, so much fail on so few lines of code. Clearly we should've had test coverage for this. Fortunately, this issue and #2628656 are now rectifying that!

Actually, the code in HEAD is correct…

I manually tested it from an end user's POV with max width 300 and no max height. The end result is a message that says it's been resized to "300x0", but the image was actually resized to 300 pixels wide and the height is calculated automatically: if the max height is 0, it is calculated to retain the same aspect ratio.

So, in fact, this is a valuable feature: you can specify max width or max height, and not care about anything else. Requiring both or neither dimensions would be a feature regression.

So, what actually needs to happen here:

1. Fix the message to be sensible in cases where only one dimension is specified (the message always says the unbounded dimension is resized to 0, which makes no sense)
2. Explicitly spell out in the config schema that width and height can be null
3. Test coverage for using EditorImageDialog:
   1. Test coverage for when both max dimensions are provided
   2. Test coverage for when neither max dimension is provided
   3. Test coverage for when either max dimension is provided
4. Test coverage for using the Text Editor configuration form:
   1. Test coverage for when both max dimensions are provided, validating the stored data
   2. Test coverage for when neither max dimension is provided, validating the stored data
   3. Test coverage for when either max dimension is provided, validating the stored data (specifically, that either dimension is null and not 0 or something else)

Some parts of the existing test can be reused to address points 4.1 and 4.2. That already provides part of what you need to do 4.3.

I've attached a patch that does point 2, because few people know the Config Schema API well. I'll leave everything else to you, thpoul, if you're up for it. I'll continue mentoring/reviewing you, of course. Sorry for having led you in the wrong direction!

On that note, to be able to points 1 and 3, you actually need max dimensions to be working. So this issue is now effectively blocked on #2628656: Max dimensions do not apply to Editor Inline image.

Oh, actually, most of point 3 can actually be done in #2628656: Max dimensions do not apply to Editor Inline image, the current patch already has the bulk of what is needed! :)

Switching to NW since #2628656: Max dimensions do not apply to Editor Inline image is now fixed :)

Indeed, and YAY!

Updating the IS to reflect which parts were already done in #2628656: Max dimensions do not apply to Editor Inline image.

Regarding point 1 I think that the problem relies on file_validate_image_resolution() which takes for granted that there will be provided either both width and height or neither of them. Also the function for some reason asks for the $image->getFileSize() without using it anywhere. I suggest we change the message from The image was resized to fit within the maximum allowed dimensions of %dimensions pixels. to something more sophisticated that actually makes sense to the user. Maybe we should let the user know the dimensions of the scaled image and not the maximum allowed ones, maybe both. Suggestions?

One more problem I found is that when I tried to upload an image (with max 100px width only set) to replicate the behavior I got the same message twice (see screenshot).

@Wim Leers: Should the test coverage on the same track like the one provided in the previous patches?

to something more sophisticated that actually makes sense to the user.

+1

Maybe we should let the user know the dimensions of the scaled image and not the maximum allowed ones, maybe both. Suggestions?

I think letting the user know that it was scaled to fit within the maximum dimensions is simplest and best. If they're sufficiently technically oriented to even care about the actual resized solution, they can inspect the image itself.

The duplicate message problem is a known bug elsewhere in the AJAX system, we don't need to address that here.

Thank you for your review Wim! Hopefully the updated patch is no the right direction and the only thing left todo is to right the relevant tests at \Drupal\image\Tests\ImageFieldValidateTest::testResolution

This looks great! Indeed the only remaining task here is updating \Drupal\image\Tests\ImageFieldValidateTest::testResolution().

This fixes points 1 (Fix the message to be sensible in cases where only one dimension is specified (the message always says the unbounded dimension is resized to 0, which makes no sense)) and points 4.1, 4.2 and 4.3:

4. Test coverage for using the Text Editor configuration form:
   1. Test coverage for when both max dimensions are provided, validating the stored data
   2. Test coverage for when neither max dimension is provided, validating the stored data
   3. Test coverage for when either max dimension is provided, validating the stored data (specifically, that either dimension is null and not 0 or something else)

In other words: once this patch lands, we have strong, comprehensive test coverage for the image upload maximum dimensions settings, as well as the expected behavior of those settings on uploaded images — awesome!

Here are a few nitpicks:

1. +++ b/core/modules/editor/src/Tests/EditorUploadImageScaleTest.php
   @@ -50,8 +50,8 @@ protected function setUp() {
   +          'width' => null,
   +          'height' => null
   
   @@ -71,9 +71,10 @@ public function testEditorUploadImageScale() {
   +    $max_width = null;
   +    $max_height = null;
   
   @@ -107,10 +110,37 @@ public function testEditorUploadImageScale() {
   +    $max_height = null;
   ...
   +    $max_width = null;
   

   Nit: s/null/NULL/

2. +++ b/core/modules/file/file.module
   @@ -444,8 +444,15 @@ function file_validate_image_resolution(FileInterface $file, $maximum_dimensions
   -          $file->filesize = $image->getFileSize();
   

   This was not being used, nice catch!

3. +++ b/core/modules/file/file.module
   @@ -444,8 +444,15 @@ function file_validate_image_resolution(FileInterface $file, $maximum_dimensions
   +            drupal_set_message(t('The image was resized to fit within the maximum allowed dimensions of %dimensions pixels.', array('%dimensions' => $maximum_dimensions)));
   ...
   +            drupal_set_message(t('The image was resized to fit within the maximum allowed height of %height pixels.', array('%height' => $height)));
   ...
   +            drupal_set_message(t('The image was resized to fit within the maximum allowed width of %width pixels.', array('%width' => $width)));
   

   Rather than calling drupal_set_message() from 3 different places, I think it'd be clearer to assign the message to $message in these 3 places, and then add a

   drupal_set_message($message);
   

   at the very end, after those if/else statements.

Making changes as suggested in #59.

Ultra-minor but sould be elseif instead of else if

#60 addressed the nits. But we still need this:

Indeed the only remaining task here is updating \Drupal\image\Tests\ImageFieldValidateTest::testResolution().

Thank you @swarad07 & @oriol_e9g for your input :)

@WimLeers: Hopefully this is close to what we need

+++ b/core/modules/image/src/Tests/ImageFieldValidateTest.php
@@ -102,4 +138,21 @@ function testRequiredAttributes() {
+  }
 }

Nit spotted :(

This looks perfect! :)

Thanks so much, now the configuration of the image upload settings and the EditorImageDialog has very solid test coverage — almost completely thanks to you, @thpoul!

+++ b/core/modules/image/src/Tests/ImageFieldValidateTest.php
@@ -102,4 +138,21 @@ function testRequiredAttributes() {
+   * @param array $min_resolution
...
+   * @param string $max_resolution

Nit: These should both be int[]. Can be fixed on commit.

Some nits :)

Looks great! :)

Patch still applies and passes the tests.

Random fail. RTBC as per Wim Leers above.

fail: [Browser] Line 248 of core/modules/system/src/Tests/Update/UpdatePathTestBase.php:
GET http://localhost/checkout/update.php/start?id=2&op=do_nojs returned 0 (0 bytes).

fail: [Other] Line 264 of core/modules/system/src/Tests/Update/UpdatePathTestBase.php:
Schema key block.block.bartik_account_menu:settings.cache failed with: missing schema
...

Given this has new strings I think this only can go into 8.2.x.

Committed fbcd924 and pushed to 8.2.x. Thanks!

Ahh, good point! I didn't think of that. Thanks!