theme_image_style_preview() and image_requirements() should check whether style directories are writable

Same reasonably clean install, same result.

Same here.

setting permissions to 777 on the filesystem's 'styles' directory where the -large -medium and -small folders should be created fixed the issue for me. Changed perms back to 755 afterward and all was still fine.

just for testing, afterwards made a new directory in /sites pointed my filesystem there and everything was ok. but no idea why the original /sites/mysite.com/files/styles directory didn't work.

@rpmute magic! :)
Thanks man.

Confirmed this bug by revoking write access on the styles directory before the style-specific subdirectory has been created (or after manually removing that directory). theme_image_style_preview ignores image_style_create_derivative's return code and does not pick up on the fact that it failed to create the subdirectory.

This is basically a configuration error (incorrect permissions set on the files (sub-)folders), hence downgrading to normal. Attached is a patch that only themes the preview if the derivative image exists or could be created. While the patch removes the notices, it does not give visual feedback about why the preview is missing (not sure if we should add an error message for this or not). image_style_create_derivative does log its message in watchdog:

Failed to create style directory: public://styles/thumbnail/public/modules/image

The real problem with this bug is that you have no idea what to do when you encounter it. Googling an error message was the only way for me, so I'd say don't hide the message (although a more meaningful one would clearly be better).

Added a meaningful message and made the patch shorter in the process.

diff --git a/modules/image/image.admin.inc b/modules/image/image.admin.inc
index d72fdf4..8d785d6 100644
--- a/modules/image/image.admin.inc
+++ b/modules/image/image.admin.inc
@@ -773,8 +773,14 @@ function theme_image_style_preview($variables) {
 
   // Set up preview file information.
   $preview_file = image_style_path($style['name'], $original_path);
-  if (!file_exists($preview_file)) {
-    image_style_create_derivative($style, $original_path, $preview_file);
+
+  if (!(file_exists($preview_file) ||
+        image_style_create_derivative($style, $original_path, $preview_file))) {
+    drupal_set_message(t('Preview image could not be created. ' .
+                         'Check permissions on your %d directory.',
+                         array('%d' => drupal_dirname($preview_file))),
+                       'warning', FALSE);
+    return '';
   }
   $preview_image = image_get_info($preview_file);
   if ($preview_image['width'] > $preview_image['height']) {

Better title.

@pillarsdotnet: thanks for picking this one up!

Regarding your patch:

• The patch seems to have some strange indentation that does not conform to the Drupal Coding Standards
• I don't think it is a good idea to use drupal_set_message() inside a theme function. Running into this problem means not having followed the installation instructions with regards to folder permissions, and there is already a fairly descriptive message logged to watchdog. If we want to offer additional info, then I think a hook_requirements() implementation is better.

Regarding "strange indentation" versus coding standards:

• Although wrapping a long conditional expression is not required, neither is it expressly forbidden.
• I believe the "indent two spaces" rule was intended for statements within an enclosing flow-control statement, not for subexpressions within the same statement. The rule should be amended for clarification.
• I believe that equally-nested parts of the same expression should be equally indented.

Personally, I prefer the following rules:

1. All statements should be wrapped at 80 characters where possible.
2. When on different lines, closing symbols should line up with their matching opening symbol.
3. Enclosed content should be indented two spaces beyond its enclosing symbols.
4. Equally-nested parts of the same expression should be equally indented.

Here is an illustration:

  if ( !( file_exists ($preview_file) ||
          image_style_create_derivative( $style,
                                         $original_path,
                                         $preview_file
                                       )
        )
     )
  { watchdog( 'theme_image_style_preview',
              'Preview image could not be created. '
              . 'Check permissions on your %d directory.',
              array('%d' => drupal_dirname($preview_file))
            );
    return '';
  }

But Drupal style forbids rule #2, so the rest become impractical.

Attached patch slavishly adheres to Drupal Coding Standards but contains a line with 147 characters. I've seen worse, but rarely produced it.

Also at your request, the patch adds the first watchdog() rather than the ninth drupal_set_message() in the file.

If the goal is to fail with no useful feedback whatsoever, this issue should be closed (won't fix).

Also at your request, the patch adds the first watchdog() rather than the ninth drupal_set_message() in the file.

Please re-read my comment: I pointed out that there already is a watchdog error logged when this situation occurs (#1020870-6: theme_image_style_preview() and image_requirements() should check whether style directories are writable), not that we should add another one...

If the goal is to fail with no useful feedback whatsoever, this issue should be closed (won't fix).

No, this issue started out as a bug about raw warnings being displayed in the interface. At a minimum that needs to be fixed. I'm ok with additional information made available, I'm just not convinced that a drupal_set_message in a theme function is the right place for it.

If both watchdog() and drupal_set_message() are unacceptable, please provide an alternative way to make additional information available.

See my suggestion in #10: other parts of core that check for writable/non-writable/folder permissions do so via hook_requirements.

For example, if you remove write permissions on sites/default/files, you'll get the following entry in the status report:

File system Writable (public download method)
The directory sites/default/files is not writable. You may need to set the correct directory at the file system settings page or change the current directory's permissions so that it is writable.

Image.module already implements hook_requirements, so we could add an additional check to verify that all folders for all image styles are writable.

Okay, created a requirement that image style directories under the file_default_scheme() be writable.

Please review...

Revised title.

I suppose the drupal_dirname() call isn't really necessary...

More bikeshedding...

Slightly shorter / clearer.

Bah. wrong file.

Bumping and tagging...

You can help this issue by posting a review:

• Decide whether all code and comments comply with Drupal coding standards.

• Decide whether any included tests are necessary and sufficient.

• Decide whether the patch actually solves the problem. There should be a reproducible test where the current code fails and the patched code succeeds.

A review that affirms success in all of the above should also:

• Change the issue status from "Needs Review" to "Reviewed and Tested By the Community".

Otherwise, the review should:

• Explain what is wrong with the proposed patch and/or supply a corrections to it.

• Change the issue status from "Needs Review" to "Needs Work".

Will this patch work on Drupal 7?

@23 yes. Right now there is no significant difference between 8.x and 7.x.

#20: 1020870-20.patch queued for re-testing.

#20: 1020870-20.patch queued for re-testing.

#20: 1020870-20.patch queued for re-testing.

There's an issue over here which is older but doesn't have as much progress on seemingly the same thing #873838: theme_image_style_preview() and image_requirements() should check whether style directories are writable.

Marking this as a duplicate and combining both patches in the older issue. Thanks Letharion!

#20: 1020870-20.patch queued for re-testing.

Confirmed bug. This seems to be related to the other bug mentioned above. Directory permissions related.

Once again, closing as a duplicate of #873838: theme_image_style_preview() and image_requirements() should check whether style directories are writable.

Very confusing when two different issues have such long and similar titles...

Sorry, Lars; that's probably my fault, too.

#20: 1020870-20.patch queued for re-testing.

Any new suggestion?

i found this bug in the past days. i had to change a hosting server. In the new server not allowed the php chmod command, for this reason my site not create the image files. I dont'understand why need drupal to change permission, when it comes from the parent.
I tried a clean drupal install, but images styles not work.

My site is on development phase, and no way to finalise it because my provider not allow chmod command in the future.

#19 worked for me.
It did change some folder permissions, but when i changed them back (+w) it seems to work fine.