numeric names fail image creation

Reassigning to core.

Looking into this.

What I've found so far is that for an image style with a numeric name (as should be allowed - the number should be treated as a string in this context) the image_style name that's serialized to the database in table "field_config_instance" for an image_style named "100" is "0" instead.

I haven't tracked this down to its root yet, but I suspect that "100" is somewhere being treated as a numeric value instead of as a string.

This needs to be fixed in D8 first and then backported

Hah! Found it!

The problem seems to be that neither array_keys() nor array_merge() preserve the data types of the array keys, so an associative key of '100' is being coerced into an int - but it starts out, and ought to remain, a string.

I haven't written any new tests for it - not really sure how, and it seems a daunting task - the setup requires creating a numerically named image style, a new image field, a node-based entity containing the image field, and setting one of the entity's views to display the image using the new (numerical-string-named) image style.

Anyway, attaching the patch ...

Thanks for the patch ...

+++ b/core/modules/image/image.moduleundefined
@@ -583,7 +583,18 @@ function image_style_options($include_empty = TRUE) {
+  // iterating through the associative array to capture the keys instead of
+  // using array_keys() because the latter loses the data type of the keys,
+  // turning associative string keys into numeric ones.
+  $keys = array();
+  foreach($styles as $key => $value) {
+      $keys[] = (string)$key;
+  }

If "adding" the two arrays (instead of array_merge) preserves the keys then can't you remove this?

+++ b/core/modules/image/image.moduleundefined
@@ -583,7 +583,18 @@ function image_style_options($include_empty = TRUE) {
+  // using the array concatenation operator '+' here instead of array_merge()

Comments should be full sentences including capitalization.

Adding tests is really not too hard and it makes a huge difference in moving your patches along in the process ... I'm happy to walk you through it in IRC #drupal (bleen)

-13 days to next Drupal core point release.

Surely this change can be simplified to just

$options = array_merge($options, drupal_map_assoc(array_keys($styles), 'strval'));

This will result in drupal_map_assoc casting to a string.

Ok my idea abvove won't work and there seems to be no way around the problem directly in php,

Yes, you're right, @bleen18 - after a re-look I saw that there was no need to replace the array_keys() call.

I'll look for you on irc for hints on writing tests...

Think I figured out the testing - at least sufficient for this purpose. Here's a patch with test code included.

Re-roll minus evil tabs :)

Thanks, @pingers. Yeah - I was just fixing that - tabs instead of spaces rears its ugly head :-(

Most of this is code styling (... I highly recommend you install Dreditor as it will help point out the white space issues when you are looking at a patch on D.O.) But the last comment below could use some more opinions...

+++ b/core/modules/image/image.testundefined
@@ -369,154 +369,164 @@ class ImageAdminStylesUnitTest extends ImageFieldTestCase {
+   * ¶

white space

+++ b/core/modules/image/image.testundefined
@@ -369,154 +369,164 @@ class ImageAdminStylesUnitTest extends ImageFieldTestCase {
+    for($run_ct = 1;$run_ct<=2;$run_ct++) {

needs a space after "for" and a space after each ";" and a space before and after "<="

+++ b/core/modules/image/image.testundefined
@@ -369,154 +369,164 @@ class ImageAdminStylesUnitTest extends ImageFieldTestCase {
+  ¶

white space

+++ b/core/modules/image/image.testundefined
@@ -369,154 +369,164 @@ class ImageAdminStylesUnitTest extends ImageFieldTestCase {
+  ¶

white space

+++ b/core/modules/image/image.testundefined
@@ -369,154 +369,164 @@ class ImageAdminStylesUnitTest extends ImageFieldTestCase {
+  ¶

white space

+++ b/core/modules/image/image.testundefined
@@ -369,154 +369,164 @@ class ImageAdminStylesUnitTest extends ImageFieldTestCase {
+  ¶

white space

+++ b/core/modules/image/image.testundefined
@@ -369,154 +369,164 @@ class ImageAdminStylesUnitTest extends ImageFieldTestCase {
+  ¶

white space

+++ b/core/modules/image/image.testundefined
@@ -369,154 +369,164 @@ class ImageAdminStylesUnitTest extends ImageFieldTestCase {
+  ¶

white space

+++ b/core/modules/image/image.testundefined
@@ -369,154 +369,164 @@ class ImageAdminStylesUnitTest extends ImageFieldTestCase {
+  ¶

white space

+++ b/core/modules/image/image.testundefined
@@ -369,154 +369,164 @@ class ImageAdminStylesUnitTest extends ImageFieldTestCase {
+  ¶

white space

+++ b/core/modules/image/image.testundefined
@@ -369,154 +369,164 @@ class ImageAdminStylesUnitTest extends ImageFieldTestCase {
+  ¶

white space

+++ b/core/modules/image/image.testundefined
@@ -369,154 +369,164 @@ class ImageAdminStylesUnitTest extends ImageFieldTestCase {
+        if ($effect_edits_order[$index] != $effect['name']) {

two spaces before the "{"

+++ b/core/modules/image/image.testundefined
@@ -369,154 +369,164 @@ class ImageAdminStylesUnitTest extends ImageFieldTestCase {
+  ¶

white space

+++ b/core/modules/image/image.testundefined
@@ -369,154 +369,164 @@ class ImageAdminStylesUnitTest extends ImageFieldTestCase {
+  ¶

white space

+++ b/core/modules/simpletest/drupal_web_test_case.phpundefined
@@ -605,23 +605,37 @@ abstract class DrupalTestCase {
   /**
-   * Generates a random string containing letters and numbers.
+   * Generates a random string.
    *
-   * The string will always start with a letter. The letters may be upper or
-   * lower case. This method is better for restricted inputs that do not
-   * accept certain characters. For example, when testing input fields that
-   * require machine readable values (i.e. without spaces and non-standard
-   * characters) this method is best.
+   * When $numeric_only is TRUE, the string will consist only of numeric digits.
+   * When $numeric_only is FALSE, the string may contain any alphanumeric value,
+   * but will always start with a letter. The letters may be upper or lower
+   * case. This method is better for restricted inputs that do not accept
+   * certain characters. For example, when testing input fields that require
+   * machine readable values (i.e. without spaces and non-standard characters)
+   * this method is best.
    *
    * @param $length
    *   Length of random string to generate.
+   * @param $numeric_only
+   *   TRUE if generated string should consist only of numbers, FALSE otherwise.
    * @return
    *   Randomly generated string.
    */
-  public static function randomName($length = 8) {
-    $values = array_merge(range(65, 90), range(97, 122), range(48, 57));
+  public static function randomName($length = 8, $numeric_only = FALSE) {
+    if($numeric_only) {
+      $values = range(48,57);
+    }
+    else {
+      $values = array_merge(range(65, 90), range(97, 122), range(48, 57));
+    }
     $max = count($values) - 1;
-    $str = chr(mt_rand(97, 122));
+    if(!$numeric_only) {
+      $str = chr(mt_rand(97, 122));
+    }
+    else {
+      $str = '';
+    }
     for ($i = 1; $i < $length; $i++) {
       $str .= chr($values[mt_rand(0, $max)]);

I think it would be much more practical to create a new function called "randomNumericString" or perhaps to just use $str = (string)rand(x,y); in your test.

Anyone else want to chime in on this one?

-14 days to next Drupal core point release.

I don't believe there is any need to make changes to public static function randomName you can simply call rand() - http://php.net/manual/en/function.rand.php

Test style can also be pretty much left intact instead I would be something like this covert the existing test style to helper function

function _testStyle($string_name = TRUE) {
  // Setup a style to be created and effects to add to it.
  $style_name = $string_name ? strtolower($this->randomName(10)) : rand();

And then in the exist testStyle function do

function testStyle() {
  _testStyle() ;
  _testStyle(FALSE) ;
}

I don't believe there is any need to make changes to public static function randomName you can simply call rand() - http://php.net/manual/en/function.rand.php

Test style can also be pretty much left intact instead I would be something like this covert the existing test style to helper function

function _testStyle($string_name = TRUE) {
  // Setup a style to be created and effects to add to it.
  $style_name = $string_name ? strtolower($this->randomName(10)) : rand();

And then in the exist testStyle function do

function testStyle() {
  $this->_testStyle() ;
  $this->_testStyle(FALSE) ;
}

The test approach above doesn't work. This test makes sure that a numeric style appears in the option list rather than being a value of 0. First patch is test only second patch is test plus fix.

This looks great!

• Patch applies
• Tests pass
• tested manually with image presets with numeric names (ex. 250)

Thanks @jmarkel & @marcingy

RTBC

@marcingy, I don't think your test exercises the functionality that is being fixed. The problem was not that the image style name did not appear in the list of available styles - it DID appear. The problem was that a style with a numeric name was not being used even when selected, because array_merge() was changing its array key value from a string type to an int typ. Because it became a numeric index in an array that was otherwise associative, the index became '0' instead of the int equivalent of the numeric style name. My test runs through the entire testStyle suite to make sure that the numeric style name is not breaking somewhere else in a similar way.

+++ b/core/modules/image/image.testundefined
@@ -368,6 +368,20 @@ class ImageAdminStylesUnitTest extends ImageFieldTestCase {
+    $this->assertTrue(array_key_exists($style_name, $options), t('Array key %key exists.', array('%key' => $style_name)));

I think this line here tests the functionality you are referring to. By asserting that the array key is returned correctly we are asserting that the root cause of the problem (the array merge mangling the keys) is no longer a problem. Once that root cause is tested then we can rely on the other tests that ensure that styles apply correctly. Make sense?

If so, you can put this back to RTBC. If not, why?

-14 days to next Drupal core point release.

Okay, thanks @marcingy and @bleen - I see it now.

There is, however a change still to be made in that the test description is incorrect (and there's a typo in it) - it should be something like
"Test to make sure that an image style with a numeric name is applied to image fields correctly."

I can re-roll in the morning unless marcingy wants to do it before then.

Okay - re-rolled with new comment for the new test.

+++ b/core/modules/image/image.testundefined
@@ -368,6 +368,21 @@ class ImageAdminStylesUnitTest extends ImageFieldTestCase {
+   * Test to create an image style with a numeric name and apply it to an image
+   * field.

How about "Test creating an image style with a numeric name and ensuring it can be applied to an image."

As requested...

Sold!! Looks good.

Thanks. Committed/pushed to 8.x, moving to 7.x for backport.

First shot at a D7 backport.

well, that seems good :)

Tested locally without the fix, test failed.
Tested locally with the fix, test passed.

nice fix, nice test!

committed and pushed to 7.x. thanks!