Terminology update: don't say "Resolution" when we mean "Dimensions"

Agreed

Tagging.

looks good

Function names should be changed too, to agree with the terminology. For example, testResolution() should be renamed testDimensions(), now that the function comments all say dimensions instead of resolution.

These are the functions I found referenced in the patch:

testResolution()
testFileValidateImageResolution()
file_validate_image_resolution()
_image_field_resolution_validate()

Tested the patch in Drupal 8 and it is looking good in user interface and code.

There is user.module.orig file in the patch which needs to be removed from the patch as per http://drupal.org/node/1268890.

Druapl 7 user interface is looking good too but this makes some changes in the code. Does it mean that patch needs to backport to Drupal 7 also?

#13: core-dimensions_not_resolution-1215784-13.patch queued for re-testing.

Changes made to user.install , reroll

D8 patch looks good!
Tested with image fields on content types and default user picture field.

I would've expected to see an upgrade path here from D7 sites using the old name? There are hunks that touch update functions, but they look like find/replace.

D8 currently migrates user pictures into image fields.

The patch in #16 updates the migration code so it still works:

@@ -731,8 +731,8 @@ function user_update_8011() {
       'max_filesize' => update_variable_get('user_picture_file_size', '30') . ' KB',
       'alt_field' => 0,
       'title_field' => 0,
-      'max_resolution' => update_variable_get('user_picture_dimensions', '85x85'),
-      'min_resolution' => '',
+      'max_dimensions' => update_variable_get('user_picture_dimensions', '85x85'),
+      'min_dimensions' => '',

#16: core-dimensions_not_resolution-1215784-16.patch queued for re-testing.

I think you are missing webchick's point here ... if the variable was named "foo_resolution" in D7, then I would expect that an upgrade to D8 would create a new variable called "foo_dimensions" and that it would get its initial value from "foo_resolution". That is not what is happening here.

At some point i would expect to see something that looks like this:

'foo_dimensions' = update_variable_get('foo_resolution', '');

And additionally, we'd need to go through all the fields and update the widget settings for all fields to rename resolution to dimensions. I'd love to see this fixed too.

The variable is called "user_picture_dimensions" so it already has the correct terminology. It gets deleted in user_update_8011() because it is migrated into field config. The patch in #16 is correct in this regard because it changes user_update_8011() so the field config that is being created uses the new config item names.

The patch in #16 is correct in this regard because it changes user_update_8011() so the field config that is being created uses the new config item names.

Ah, clever. Well that means a definite breakage of existing 8.x sites. I'm not sure where this fits in our policies. I think it's basically, "make 8.x-to-8.x upgrades work if it's easy, but breaks are okay if upgrades are hard". That seems to describe this situation, so maybe no new updates are needed.

Rerolled.
Changes in core/profiles/standard/standard.install got moved into core/profiles/standard/config/field.instance.node.article.field_image.yml.

the patch...

I am seeing, (as quicksketch suggests in #23) that on D7 => D8 upgrade, the field instance settings haven't been changed.

Notice: Undefined index: max_dimensions in image_field_instance_settings_form() (line 100 of core/modules/image/image.field.inc).
Notice: Undefined index: min_dimensions in image_field_instance_settings_form() (line 127 of core/modules/image/image.field.inc).

I'm not yet sure where this should get updated.

Issue summary stuff, better.

rerolled the patch.

I have completed the terminology changes and rolled a new patch.

Two API changes to note:
1. The function "file_validate_image_resolution" has become "file_validate_image_dimensions"
2. The method "validateResolution" in class imageItem has changed to "validateDimensions"

The Interdiff file is also attached.

I realised that I did not create the interdiff for patch #33 correctly, so have re-done it properly.

Patch re-rolled against latest 8.0.x HEAD.

Patch re-rolled against latest drupal-8.0.x.

Is this a "beta deadline" issue?

No not really, there's one API change in the patch:

-function file_validate_image_resolution(FileInterface $file, $maximum_dimensions = 0, $minimum_dimensions = 0) {
+function file_validate_image_dimensions(FileInterface $file, $maximum_dimensions = 0, $minimum_dimensions = 0) {

That's not going to affect much, so I'd allow it during early beta. We could leave the old function as a wrapper and mark it @deprecated past that if we need to.

However it changes configuration, so it does need to be 'beta target'/'D8 upgrade path'. If it doesn't get committed before we start the upgrade path, then either:

1. We'd have to skip the bits touching configuration.

2. We'd have to write an upgrade path. Depending on what the upgrade path looks like, that could also mean the issue being delayed until 8.1.x or 9.x if it might be disruptive.

Looks like this needs a reroll now that #657166: use × instead of x is in.

Patch has been rerolled against the latest 8.0.x HEAD.

I checked the patch for D8 :
- in code every reference to "resolution" has been replaced.
- in UI, everything looks OK now.

To develop what catch said in #45, we should make the old function a wrapper containing the new function (marked as deprecated), so it will up to everybody which one they want to use. We won't break anything doing this.

Here is the patch

@leonrenkema sorry I forgot to assign the issue to myself

This issue will be reviewed after the test comes back.

The wrapper works and the old function is now indicated as deprecated. RTBC.

Should the other API change:

- method rename (in class ImageItem):
public static function validateResolution($element, &$form_state) is renamed to "validateDimensions"

also be made into a wrapper and marked as deprecated?

My bad! This API change has disappeared in one of the recent core changes. Just ignore this comment :)

Looks like a random/unrelated fail. Only one remark on the patch:

+++ b/core/modules/file/file.module
@@ -426,6 +426,13 @@ function file_validate_is_image(FileInterface $file) {
+ * @deprecated file_validate_image_dimensions()

Our docs standards at https://www.drupal.org/coding-standards/docs#deprecated suggests a one line explanation and @see to point to the one to use instead.

Rerolled, conflicted with #2363077: Max and min resolution not working. Removed assigned field as it has been a long time without work here.

Fixed #61 and fixed phpdocs in file_validate_image_dimensions().

Found more occurrences. This should be good to go. Take into account that there are some references, but they are in migration sources so they must not be modified.

Oops! s/dimension/dimensions/g

1. +++ b/core/modules/file/file.module
   @@ -399,6 +399,16 @@ function file_validate_is_image(FileInterface $file) {
    /**
   + * @deprecated in Drupal 8.0.x-dev, will be removed before Drupal 9.0.
   + *   Use file_validate_image_dimensions() instead.
   + *
   + * @see file_validate_image_dimensions()
   + */
   

   Docs standard is that the first line should be a brief explanation of the function, followed by more detailed information. I'd suggest:

   /**
   * Backwards compatibility wrapper for file_validate_image_dimensions().
   *
   * @deprecated in Drupal 8.0.0, will be removed for Drupal 9.0.0.
   *
   * @see file_validate_image_dimensions()
   */

   I've also tweaked the @deprecated line, as discussed in #2158871: [Policy, no patch] Clearly denote when @deprecated code is slated to be removed.

2. +++ b/core/modules/file/file.module
   @@ -406,22 +416,22 @@ function file_validate_is_image(FileInterface $file) {
   - * @param $maximum_dimensions
   + * @param int $maximum_dimensions
     *   An optional string in the form WIDTHxHEIGHT e.g. '640x480' or '85x85'. If
     *   an image toolkit is installed the image will be resized down to these
     *   dimensions. A value of 0 indicates no restriction on size, so resizing
     *   will be attempted.
   - * @param $minimum_dimensions
   + * @param int $minimum_dimensions
     *   An optional string in the form WIDTHxHEIGHT. This will check that the
     *   image meets a minimum size. A value of 0 indicates no restriction.
   

   Doesn't seem necessary to add the parameter types here, but if we're going to then they should be string (see the descriptions).

3. +++ b/core/modules/file/file.module
   @@ -406,22 +416,22 @@ function file_validate_is_image(FileInterface $file) {
   - * @return
   + * @return array
     *   An array. If the file is an image and did not meet the requirements, it
     *   will contain an error message.
   

   This should be mixed, because it might return an error string.

4. +++ b/core/modules/file/src/Tests/ValidatorTest.php
   @@ -63,24 +63,24 @@ function testFileValidateIsImage() {
   +   *  This ensures the dimensions of a specific file is within bounds.
   

   ARE within bounds, not is.

5. I've restored the validateResolution > validateDimensions line that @jlscott marked as deleted in the issue summary, as the patch is changing that API (as it should). As we've got a BC wrapper for the function, we should have a wrapper for the method too.

Here is my first patch, please review.

Fix tag. Please announce at your sprint that the right tag does not include a #.

Great to see a new contributor - thanks for your patch. Unfortunately looking at your interdiff, you've broken the backwards compatibility wrapper and undone one of the previous changes from resolution to dimensions, neither of which are changes we want. There are no other changes, and #70 has not been addressed. If you give your interdiff a .txt extension, then that will stop it being picked up by the testbot.

Any further patches should be based off #66.

I tried to use @penyaskito patch but it doesn't apply... :(
I think I didn't forget change any image resolution...

Thanks for your patch!
Unfortunately you've broken the backwards compatibility wrapper. We need to maintain that.

See this from #66:

  /**
+ * @deprecated file_validate_image_dimensions()
+ */
+function file_validate_image_resolution(FileInterface $file, $maximum_dimensions = 0, $minimum_dimensions = 0) {
+  return file_validate_image_dimensions($file, $maximum_dimensions, $minimum_dimensions);
+}
+
+/**

Bad patch!

test patch 2

This is the correct second patch. I apologize for my noob uploads before...

Thanks for your perseverance! See the docblock documentation at #70 for the deprecation notice and use it, please.

Done! and thank you for your time, I appreciate it ^^

+++ b/core/modules/file/src/Tests/ValidatorTest.php
@@ -63,24 +63,24 @@ function testFileValidateIsImage() {
+  function testFileValidateImageiDimensions() {

Extra i.

+++ b/core/modules/file/src/Tests/ValidatorTest.php
@@ -63,24 +63,24 @@ function testFileValidateIsImage() {
+   *  This ensures the dimensions of a specific file is within bounds.

*are* within bounds.

See extra comments at #70, I think we are missing some of those again.

Fixed extra "i", documentation and added backward compatibility

Go, testbot!

I think this is ready if green.

We need a change record for the schema changes and I think it would be good to inform everyone of the change in terminology.

1. +++ b/core/modules/migrate_drupal/config/install/migrate.migration.d6_user_picture_field_instance.yml
   @@ -22,7 +22,7 @@ process:
   -  'settings/max_resolution': max_resolution
   +  'settings/max_dimensions': max_dimensions
   
   +++ b/core/modules/migrate_drupal/src/Plugin/migrate/process/d6/FieldInstanceSettings.php
   @@ -56,8 +56,8 @@ public function transform($value, MigrateExecutable $migrate_executable, Row $ro
   -        $settings['max_resolution'] = $widget_settings['max_resolution'];
   -        $settings['min_resolution'] = $widget_settings['min_resolution'];
   +        $settings['max_dimensions'] = $widget_settings['max_dimensions'];
   

   Source shouldn't have changed, these represents the D6 stored data.

2. +++ b/core/modules/migrate_drupal/src/Tests/Table/ContentNodeFieldInstance.php
   @@ -257,7 +257,7 @@ public function load() {
   -      'widget_settings' => 'a:14:{s:15:"file_extensions";s:16:"png gif jpg jpeg";s:9:"file_path";s:0:"";s:18:"progress_indicator";s:3:"bar";s:21:"max_filesize_per_file";s:0:"";s:21:"max_filesize_per_node";s:0:"";s:14:"max_resolution";s:1:"0";s:14:"min_resolution";s:1:"0";s:3:"alt";s:8:"Test alt";s:10:"custom_alt";i:0;s:5:"title";s:10:"Test title";s:12:"custom_title";i:0;s:10:"title_type";s:9:"textfield";s:13:"default_image";N;s:17:"use_default_image";i:0;}',
   +      'widget_settings' => 'a:14:{s:15:"file_extensions";s:16:"png gif jpg jpeg";s:9:"file_path";s:0:"";s:18:"progress_indicator";s:3:"bar";s:21:"max_filesize_per_file";s:0:"";s:21:"max_filesize_per_node";s:0:"";s:14:"max_dimensions";s:1:"0";s:14:"min_dimensions";s:1:"0";s:3:"alt";s:8:"Test alt";s:10:"custom_alt";i:0;s:5:"title";s:10:"Test title";s:12:"custom_title";i:0;s:10:"title_type";s:9:"textfield";s:13:"default_image";N;s:17:"use_default_image";i:0;}',
   

   This shouldn't have changed because it's the D6 db dump.

Created change record draft: https://www.drupal.org/node/2423061.

Fix test

OMG your avatar is AMAZING. :D Steampunk Druplicon? :)

I forgot interdiff :(
@webchick yes!! it's http://suntog.deviantart.com work. I fall in love when I saw it <3

Hidden fail test found.

Fix source d6

I remake the patch

Great work of patch #113 by Saphyel. Patch worked perfectly on D8 and on PHP 5.6.3 with UI updated. Screenshot of UI after applying patch is attached with comment.

Altered comment #115 by adding two files as attachment. One image named as before_applying_patch.png is the screenshot of D8 UI before applying patch mentioned in comment #113 and another image named as after_applying_patch.png is the screenshot of D8 UI after applying patch.

The disruption part of the beta evaluation is incorrect - existing image fields need to be updated and whilst we are not yet support head to head upgrades we should at least be honest about this. Actually I think the configuration key renames are not worth it at this point. We break existing sites for little reason. I think this issue should be postponed to 9.x.

#alexpott but why this is so? why it shouldn't be included in Drupal 8 version inspite that patch is working completely fine?

I am removing the Novice tag from this issue because it is uncertain if this should be postponed or not.

I’m using this documentation as a source: https://www.drupal.org/core-mentoring/novice-tasks#avoid

@Shivam, the main issue here appears to be that the beta evaluation does not accurately reflect the disruption caused by the patch.

See: https://www.drupal.org/contribute/core/beta-changes#disruption (the last 2 points are particularly relevant)

@alexpott is challenging the value of the fix against the disruption that it will cause particularly to those currently hosting D8 beta sites.

It is our responsibility to put a case forward for the value of this patch.

This issue was marked as a beta target for the 8.0.x beta, but is not applicable as an 8.1.x beta target, so untagging.

This change includes some disruption, so moving to 8.2.x. It's also way more than just interface text in the current patch. One could split the UI changes from a followup for the variable and machine name changes to address this.

Getting error on applying the patch '1215784-dimensions-113.patch' in #113 on Drupal 8.4.x site.
This needs to be reworked.

Updating tag.

This issue is still very much valid and came up in the last Usability meeting #3395398: Drupal Usability Meeting 2023-10-27 as we reviewed an issue on resizing image. This should be done everywhere: replace incorrect "resolution" with the correct "dimensions" especially for all user facing texts.

I wonder of this could be split to 2 issues to provide the first easier fix faster?

1. Fix the user facing text
2. Change the variable names and other related code

I removed some of the tags that listed old events.

Splitting the variable / API changes to a separate issue was recommended by @xjm in #122 so that seems like a good way to proceed here.

I made updates to comments and user interface changing the word resolution to dimensions where relevant.
This does not change wording where referencing Drupal 6 terms (migration).
Does not update any variables and that would be a follow-up issue.

Opened #3399094: Update variable/API terminology to not use "Resolution" so removing "Needs follow up" tag
Since API changes were moved to follow up removing upgrade path and change record tag. Believe the attach CR on this issue could probably be deleted.

String freeze and minor version target I don't recognize but don't think they fully apply.

Looking at MR 5227 I only see label and comment changes so seems in scope of this issue.

Assuming the MR is canonical here since the patch files are very old. Please remember to hide patch files when converting a patch to an MR. Thanks!

Saving issue credits. (That took awhile.)

• I reviewed locally with git diff --color-words and verified that all the changes are updating code comments or UI strings where "resolution" is incorrectly used for image dimensions.
   

• I grepped for:

  grep -ri "resolution" * | grep "//" | grep -v "vendor" | grep -v "node_modules"
  

  I verified that there were no other inline comments incorrectly referring to image dimensions as "resolution".
   

• I grepped for:

  grep -ri "resolution" * | grep -v "vendor" | grep -v "node_modules" | grep '\*'
  

  ...to check for the same thing in docblocks. Doing so, I found the following files where the word "resolution" is used to mean "dimensions" in the docblock summaries for several test methods:

  core/modules/file/tests/src/Kernel/Plugin/Validation/Constraint/FileImageDimensionsConstraintValidatorTest.php
  core/modules/file/tests/src/Kernel/LegacyValidatorTest.php
  core/modules/image/tests/src/Kernel/ImageItemTest.php
  

  Renaming the parameters or test methods themselves is scoped to the followup, but the docblocks should be updated now.
   

• I grepped for:

  grep -ri "resolution" * | grep -v "vendor" | grep -v "node_modules" | grep "\bt("
  

  ...to check for obvious translatable strings containing the word. Image resolutions are mentioned in image_help(), but in that case it's maybe correct? Please evaluate whether you think this should be changed or not:

  As an example, you might upload a high-resolution image with a 4:3 aspect ratio, and display it scaled down, square cropped, or black-and-white (or any combination of these effects).

  Do they simply mean a large image, or do they mean a high-resolution image?

NW to at least fix the three test docblocks. Thanks!

I updated the 3 tests for word "dimensions".

I had deliberately left out changing the "high-resolution" part since it makes sense if read according to for instance Adobe's definition
"High resolution images are pictures or photos where the media has higher concentrations of pixels or dots, resulting in better quality and clarity of the image – as it contains more detail."

A number of end users deal with high-resolution photographs and the help-text would make sense to them. In the past the industry used terms like "press quality", "print quality" or "web quality" and here press & print would be high-resolution and web quality low-resolution. The help text communicates that Drupal handles original high-resolution photos just fine.

Appears feedback for the 3 dockblocks has been addressed.

Thanks @simohell; that explanation works for me.

Committed to 11.x and 10.2.x as it includes minor-only updates to UI strings. Thanks!

I went to publish the CR, but then realized it should actually be about the API and data model changes, not the simple string changes. So I removed it from this issue and reattached it to the followup instead.