Problem

  • When switching from the default PhpStorage\MTimeProtectedFileStorage to PhpStorage\FileStorage, you get this fatal error:
    Warning: file_put_contents(sites/default/files/php/service_container/service_container_prod_.php): failed to open stream:
    
    Permission denied in Drupal\Component\PhpStorage\FileStorage->save()
    (line 60 of core\lib\Drupal\Component\PhpStorage\FileStorage.php).
    
    Drupal\Component\PhpStorage\FileStorage->save('service_container_prod_.php', '...
    

Cause

  • sites/default/files/php/service_container/service_container_prod_.php is a directory.

Proposed solution

  1. Change MTimeProtectedFileStorage to not use a .php file suffix for its directory.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

sun’s picture

Issue tags: +Sunrise Sanity Cruise
sun’s picture

Status: Active » Needs review
FileSize
1004 bytes

Attached patch fixes this issue.

Status: Needs review » Needs work

The last submitted patch, drupal8.phpstorage-directory-name.3.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
1.92 KB

Glad to see that there are tests :)

jibran’s picture

Status: Needs review » Needs work

The last submitted patch, 5: drupal8.phpstorage-directory-name.5.patch, failed testing.

sun’s picture

Assigned: Unassigned » sun
Issue summary: View changes
Status: Needs work » Needs review
FileSize
2.03 KB

This one should come back green.

znerol’s picture

+++ b/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFastFileStorage.php
@@ -152,6 +152,13 @@ protected function getFullPath($name, &$directory = NULL, &$directory_mtime = NU
    * Returns the full path of the containing directory where the file is or should be stored.
    */
   protected function getContainingDirectoryFullPath($name) {
+    // Remove the .php file extension from the directory name. At least on
+    // Windows, a directory cannot have the same name as a file in the same
+    // folder, so when switching between MTimeProtectedFastFileStorage and
+    // FileStorage, the subdirectory cannot be created.
+    if (substr($name, -4) === '.php') {
+      $name = substr($name, 0, -4);
+    }
     return $this->directory . '/' . str_replace('/', '#', $name);

We can safely leave out the reference to Windows, other OSes behave the same.

Instead of special-casing files with php extension, wouldn't it make sense to simply substitute the periods (we already do the same with slashes). E.g:

return $this->directory . '/' . strtr($name, array('/' => '#', '.' => '^'));
sun’s picture

Adjusted the comment.

I'd prefer to keep the removal of the .php extension, because I think it's strange/awkward to have any kind of extension in a directory name to begin with (regardless of whether it's .php or #php).

Not sure about the concern of "special-casing" .php... — Technically PhpStorage only creates .php files, because that's it's only purpose. I only included the condition, because IIRC one of the unit tests writes/dumps a file without extension. If you feel strongly about that, then we could change the condition to use strrpos($name, '.') instead, so as to cut off any possibly existing file extension from the directory name.

znerol’s picture

Status: Needs review » Reviewed & tested by the community

I'd prefer to keep the removal of the .php extension

Ok, I'm fine with that. In fact the documentation on PhpStorageInterface suggests that file names are expected to have a .php suffix.

I've reproduced the failure condition and verified the fix. This is good to go.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 241356f and pushed to 8.x. Thanks!

  • Commit 241356f on 8.x by alexpott:
    Issue #1933636 by sun: PhpStorage\FileStorage tries to write a file...

Status: Fixed » Closed (fixed)

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