Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Tor Arne Thune’s picture

Version: 7.x-dev » 8.x-dev
Component: image system » user interface text

Still a valid issue. See for example the description text for a user picture upload form. Moving to 8.x.

droplet’s picture

subscribe

Brandonian’s picture

subscribe

Haza’s picture

FileSize
10.4 KB

Here are some changes about that. I also change the test according to the new × instead of x

Haza’s picture

Status: Active » Needs review
droplet’s picture

why it's using str_replace ??

Haza’s picture

Because the informations is still save with the x (the letter), not the × so the only purpose of this replace is to display a × instead of the x when displaying the data. (but I know, that still might be a bad idea ... )

Damien Tournoud’s picture

Status: Needs review » Needs work
-    watchdog('image', 'Image scale failed using the %toolkit toolkit on %path (%mimetype, %dimensions)', array('%toolkit' => $image->toolkit, '%path' => $image->source, '%mimetype' => $image->info['mime_type'], '%dimensions' => $image->info['width'] . 'x' . $image->info['height']), WATCHDOG_ERROR);
+    watchdog('image', 'Image scale failed using the %toolkit toolkit on %path (%mimetype, %dimensions)', array('%toolkit' => $image->toolkit, '%path' => $image->source, '%mimetype' => $image->info['mime_type'], '%dimensions' => $image->info['width'] . '×' . $image->info['height']), WATCHDOG_ERROR);

This (and others) look wrong. The % in %dimensions means that the argument is plain-text. As consequence it should not contain HTML. Moreover, we try to avoid using entities, and prefer straight Unicode characters.

Haza’s picture

Status: Needs work » Needs review
FileSize
10.34 KB

Got it, what about this one so ?

TR’s picture

FileSize
10.34 KB

Reposting patch from #9 without modification, in attempt to get the testbot to run.

Status: Needs review » Needs work

The last submitted patch, 657166_2.patch, failed testing.

TR’s picture

Status: Needs work » Needs review
FileSize
10.38 KB

Re-rolled patch against current D8 head, and in -p1 format for the testbot.

irunflower’s picture

Status: Needs review » Needs work

Patch did not work. Could not get it to apply to 8.x-dev.

I get: fatal: corrupt patch at line 139

heaths1’s picture

Status: Needs work » Needs review
FileSize
9.08 KB

Rerolled to the recent head. Noob input.

enhdless’s picture

Issue summary: View changes
Status: Needs review » Needs work

Patch needs rerolling again.

TR’s picture

Status: Needs work » Needs review

14: 657166-14.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 14: 657166-14.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
Issue tags: +Needs reroll
FileSize
2.88 KB

Ok, this needs a bunch of work. Where is the image_resize_effect() function? I could only find the test:

grep -ir image_resize_effect core/modules/
core/modules//image/src/Tests/ImageEffectsTest.php: * Test the image_resize_effect() function.

same for function image_crop_effect() & image_scale_and_crop_effect().

or for that matter core/modules/user/lib/Drupal/user/AccountFormController.php

This still needs work obviously, but wanting the bot to test it.

tstoeckler’s picture

Status: Needs review » Needs work
+++ b/core/modules/file/file.field.inc
@@ -185,8 +185,8 @@ function template_preprocess_file_upload_help(&$variables) {
+    $max = str_replace('x', '×', $upload_validators['file_validate_image_resolution'][0]);
+    $min = str_replace('x', '×', $upload_validators['file_validate_image_resolution'][1]);

I think that should not be necessary.
Instead everywhere where 'x' is still used we should use the proper character instead. I could find the following places that still need to updated:
EditorImageDialog
ImageItem (there's some instances of x there)

mgifford’s picture

Status: Needs work » Needs review
FileSize
5.82 KB

Ok, I dropped the str_replace() as requested and added it to EditorImageDialog & ImageItem. Let me know if I missed anything.

Status: Needs review » Needs work

The last submitted patch, 20: the-proper-x-657166-20.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
5.82 KB

Hope this addresses the failed test in core/modules/image/src/Tests/ImageFieldDisplayTest.php

Status: Needs review » Needs work

The last submitted patch, 22: the-proper-x-657166-22.patch, failed testing.

cs_shadow’s picture

This should take care of the tests.

cs_shadow’s picture

Status: Needs work » Needs review
TR’s picture

You forgot to change one of the "WIDTHxHEIGHT" that appears in the patch.

mgifford’s picture

Ok, this should address that. Thanks for pointing it out TR.

Status: Needs review » Needs work

The last submitted patch, 27: use_instead_of_x-657166-27.patch, failed testing.

cs_shadow’s picture

Seems like a very unrelated failure to me.

Status: Needs work » Needs review

mgifford’s picture

I posted a version of the code on SimplyTest.me - admin/structure/types/manage/article/fields/node.article.field_image

Image Field

I noticed it isn't in admin/config/media/image-styles when I was looking at this just now... That's probably the biggest place where it is visible in the UI.

mgifford’s picture

Now with changes to the image module.

mgifford’s picture

Sorry for the noise. Missed the YML file.

mgifford’s picture

and small/medium too apparently..

mgifford’s picture

Screenshot with × Images with ×

The last submitted patch, 33: use_instead_of_x-657166-33.patch, failed testing.

The last submitted patch, 34: use_instead_of_x-657166-34.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 35: use_instead_of_x-657166-35.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
10.93 KB

With tests..

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Awesome, thanks! The patch is now a simple search and replace, which is how it should be. The various test failures show that this is sufficiently tested.

Wim Leers’s picture

Wonderful :)

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 1cbe233 on 8.0.x
    Issue #657166 by mgifford, TR, Haza, cs_shadow, heaths1: Use × instead...
mgifford’s picture

Issue summary: View changes
yannickoo’s picture

Almost 5 years now, wow :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

alexpott’s picture