Getting a PDOException when adding new image style named thumbnail, medium and large

I can reproduce this

There was a bug on a bitwise operator that didnt handle the case where a default imagestyle is overridden. I dont know if my patch is the best approach for this.

Accidently removed the test for the include variable..

Are there perhaps constants for the 1 and 2 values that could be used here?

Also, I think this if() expression is a candidate for getting split up, per our standards on long lines: https://drupal.org/coding-standards#linelength

Some code comments to explain what each piece of the expression means would help make it understandable and more maintainable in future.

I found a new approach for this, because we actually want to give a chance to override those default image styles, so why not to test that one.

I also clarified the comments.

#6: pdoexception-error-2060235-6.patch queued for re-testing.

I verified that the patch avoids the problem described in the bug, but it does not appear right.

The point of the $include parameter is to be able to filter styles based on their storage attribute. You simply removed that test and then hardcoded a test for !IMAGE_STORAGE_DEFAULT. This must affect other places that call image_style_load with a specified $include argument.

Avoiding double negatives in the comment is probably also an improvement; say "allow" instead of "not prevent".

I belive this is a better approach. This just adds a custom function to check if the machine name is already taken.

I updated comments on the patch.

The $include parameter is the variable passed to the function in the admin interface, so only thing it would prevent is editing a default image styles, which is not possible. In other cases there shouldnt be any reason for specifing $include argument.

BTW has anyone checked to see whether this is ok on D8?

The comment should say something like:

If a style was found, only return it if its storage type matches a restriction set by the $include parameter.

> The $include parameter is the variable passed to the function in the admin interface, so only thing it would prevent is editing a default image styles, which is not possible. In other cases there shouldnt be any reason for specifing $include argument.

I'm not sure I follow. If the $include param doesn't do anything, should it be removed? The latest version of the patch doesn't compare the style with $include -- isn't it supposed to do that?

Actually Gisles #9 approach is better, and uses the $include as it should be used, and then we can save the image_style_load() as it is.

BTW has anyone checked to see whether this is ok on D8?

This seems to be working correctly in D9, I was not able to simulate the problem there.

The behavior in D7 is buggy:

• If you override one of the default styles and then try to create a new style with the same name, you will get an error mentioned above (patch #9 solves this).
• If you do not override the default style but try to create a new style with the same name anyway, the original default style gets overriden silently (which I think is not the best UX and I am curious if this is a bug or feature).

I like the approach in #9, but what if we use:

return image_style_load($name, NULL, IMAGE_STORAGE_NORMAL | IMAGE_STORAGE_OVERRIDE | IMAGE_STORAGE_DEFAULT);

instead of:

return image_style_load($name, NULL, IMAGE_STORAGE_EDITABLE);

This will also fix the usecase where the default style is not overriden and is overrided silently in the background when the new style with the same name is saved.

Anyway, I have created a test for this, so hopefully this can move forward one way or another :) Reuploaded patch also contains my proposed change in image_style_add_form_name_exists().

Adding a tag for the feedback from other maintainer.

Perhaps I'm missing something, but isn't passing all of the constants in the $include param the same as not passing it at all?

I think this patch would most likely be fine to commit, but I don't think the $include param is doing anything as it stands so it might as well be omitted?

I'm also being slightly distracted by the fact that the docblock seems to be quite wrong / outdated; it doesn't mention that $include can be a bitmask and it says it'll return an empty array if the style's not valid but it actually returns FALSE.

I'm also being slightly distracted by the fact that the docblock seems to be quite wrong / outdated; ... it says it'll return an empty array if the style's not valid but it actually returns FALSE.

Yes, this is interesting. Looking at other functions calling image_style_load(), it seems like most of them expects an array (like the documentation mentions), and not doing some extra checks. I tried to check the issue queue, if there is any complaint about this from the past, but was not able to find anything.

it doesn't mention that $include can be a bitmask

I think I figured it this out only when looked at the constants definition (when working on patch #15):

/**
 * Image style constant to represent an editable preset.
 */
define('IMAGE_STORAGE_EDITABLE', IMAGE_STORAGE_NORMAL | IMAGE_STORAGE_OVERRIDE);

Perhaps I'm missing something, but isn't passing all of the constants in the $include param the same as not passing it at all?

This is true, it seems like I have missed this. I can update the patch to remove the constants, but the main question is, do we want to get with the safer approach (patch #9, which will fix the error, but does not touch the situation, when it is possible to silently override the default style by creating a new with the same name) or we will use the more complex approach (patch #15, which will check also not-overriden styles). If we choose the more complex approach, it would probably be better to mention this at least in release notes, as this seems like a bug, but we are introducing a slight change in the behavior (though I am not sure if this can have a usefull use-case).

So I will update the patch #15 or #9, according to the decision. Thanks!

I think let's try with simply:

+/**
+ * Check if the proposed machine name is already taken.
+ */
+function image_style_add_form_name_exists($name) {
+  return (bool) image_style_load($name);
+}

The new test passes for me locally with this, but would be good to check the rest of the test suite.

Updated the patch. I am not uploading a test-only patch, as the tests have not changed (so the one from #15 is still the same).

Looks good to me!

Thanks everyone! Committed.