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

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.

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