The method Drupal\Component\PhpStorage\FileStorage does not return anything but the documentation says it does:
/**
* Ensures the directory exists, has the right permissions, and a .htaccess.
* ... snip ...
* @return bool
* TRUE if the directory exists or has been created, FALSE otherwise.
*/
protected function ensureDirectory($directory, $mode = 0777) {
if ($this->createDirectory($directory, $mode)) {
$htaccess_path = $directory . '/.htaccess';
if (!file_exists($htaccess_path) && file_put_contents($htaccess_path, static::htaccessLines())) {
@chmod($htaccess_path, 0444);
}
}
}
The return value of this method is used in the following code:
public function save($name, $code) {
$path = $this->getFullPath($name);
$directory = dirname($path);
if ($this->ensureDirectory($directory)) {
$htaccess_path = $directory . '/.htaccess';
if (!file_exists($htaccess_path) && file_put_contents($htaccess_path, static::htaccessLines())) {
@chmod($htaccess_path, 0444);
}
}
return (bool) file_put_contents($path, $code);
}
As the results, the code inside of if () { } statement is unreachable.
Fix documentation and remove unreachable code.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | interdiff.txt | 529 bytes | denutkarsh |
| #12 | 2505793-12.patch | 1.17 KB | denutkarsh |
| #8 | 2505793-8.patch | 1.13 KB | denutkarsh |
| #2 | 2505793-02.patch | 2.48 KB | jhedstrom |
Comments
Comment #1
jhodgdonYou're right! Wow, that seems like more than a Documentation bug, since the (missing) return value is (erroneously) being used in another function.
Moving to File subsystem for action, and since this an API function, and a Component even, and the save() function is really never going to work right, it may even be Major.
Comment #2
jhedstromThis also removes the duplicate code in the
save()method (that was never called previously, but would now simply repeat what happens inensureDirectory()).Comment #6
20th commented8.3.x still tries to use nonexistent return value from
Drupal\Component\PhpStorage\FileStorage::ensureDirectory. Patch need reroll.Cannot agree, however, that this is a major bug because it existed for so long without causing any obvious troubles.
Comment #7
denutkarsh commented@20th I am rerolling the patch for 8.3.x as it is persisting for it too.
Comment #8
denutkarsh commentedHere is the patch i rerolled for drupal 8.3.x .
Comment #10
denutkarsh commentedComment #12
denutkarsh commentedComment #13
denutkarsh commentedComment #14
20th commentedThis patch can be applied both to 8.2.x and 8.3.x branches, fixes documentation of
ensureDirectory()method and eliminates unreachable code insave()method.No functionality is lost because the removed code still exists inside of
ensureDirectory()method, so the.htaccessfile is still being written. Likewise, no functionality is added, so no new tests are required.It might be good to add a test for the case when the
ensureDirectory()actually fails to create directory. But then thefile_put_contents()will fail as well and that will be caught byDrupal\Tests\Component\PhpStorage\FileStorageTest::testCRUD().Comment #15
alexpottYes this is dead code. But there is no specific bug here. The what happens at run-time is just the same before and after this patch. Therefore this is a task.
Comment #16
alexpottCommitted dad2a50 and pushed to 8.3.x. Thanks!
Comment #19
denutkarsh commented