- Issue created by @mondrake
- Status changed to Needs review
6 months ago 8:47pm 11 March 2024 - Status changed to Needs work
6 months ago 11:19pm 11 March 2024 - Status changed to Needs review
6 months ago 5:13am 12 March 2024 - ๐ฎ๐นItaly mondrake ๐ฎ๐น
@smustgrave if itโs agreed this is the right approach to solve the problem, then IMO before commit we need to have two MRs in this issue, one for 10.3 (this one in its current status) and one for 11 that makes the change straight. What do you think?
- ๐ฌ๐งUnited Kingdom alexpott ๐ช๐บ๐
+1 to throwing an exception. createDirectory is only called by \Drupal\Component\PhpStorage\FileStorage::ensureDirectory() - if this triggers a warning then it ensureDirectory is not really living up to its name. Throwing an exception is the correct thing to do here.
- Assigned to mondrake
- Status changed to Needs work
6 months ago 10:55am 12 March 2024 - ๐ฎ๐นItaly mondrake ๐ฎ๐น
Grand. Let's have the two MRs, then. On it.
- ๐ฌ๐งUnited Kingdom longwave UK
+1 for this approach, I don't even think we need the extra comments in the 10.3.x version as we will commit the 11.x and 10.3.x versions simultaneously from this issue.
- Issue was unassigned.
- Status changed to Needs review
6 months ago 1:46pm 12 March 2024 - Status changed to Needs work
6 months ago 3:07pm 12 March 2024 The Needs Review Queue Bot โ tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide โ to find step-by-step guides for working with issues.
- Status changed to Needs review
6 months ago 5:38pm 12 March 2024 - Status changed to RTBC
6 months ago 7:07pm 12 March 2024 - Status changed to Needs work
6 months ago 9:27am 13 March 2024 - ๐ฌ๐งUnited Kingdom alexpott ๐ช๐บ๐
I think the changes here are going to eat errors that would be helpful for a developer. Before you'd get two warnings - one we trigger and one from file_put_contents... now you'll get nothing.
- ๐ฌ๐งUnited Kingdom alexpott ๐ช๐บ๐
asserts don't work - it's about environment - if a twig php directory is unwritable in production you don't want no information about this.
- ๐ฌ๐งUnited Kingdom alexpott ๐ช๐บ๐
I think the best thing here might be to add our own error handler in the test of the warning and be done.
- ๐ฌ๐งUnited Kingdom alexpott ๐ช๐บ๐
We could make the test something like this:
/** * @covers ::createDirectory */ public function testCreateDirectoryFailWarning() { $directory = new vfsStreamDirectory('permissionDenied', 0200); $storage = new FileStorage([ 'directory' => $directory->url(), 'bin' => 'test', ]); $code = "<?php\n echo 'here';"; $messages = []; set_error_handler(function(int $errno, string $errstr) use (&$messages){ $messages[$errno] = $errstr; }); $storage->save('subdirectory/foo.php', $code); restore_error_handler(); $this->assertCount(2, $messages); $this->assertSame('mkdir(): Permission Denied', $messages[E_USER_WARNING]); $this->assertStringStartsWith('file_put_contents(vfs://permissionDenied/test/subdirectory/foo.php)', $messages[E_WARNING]); }
Because to be honest the mkdir warning is useless. The warning that's actaully useful is the file_put_contents() one that's not even being addressed here and is silently ignored by our current test.
- ๐ฎ๐นItaly mondrake ๐ฎ๐น
#18 but then @alexpott @longwave we are back to the discussion whether we want to backfill expectError*/expectWarning* in our test framework, which I thought it was over. It's easy to do in ๐ Upgrade PHPUnit to 10, drop Symfony PHPUnit-bridge dependency Needs work , where we need to add our own error handler anyway because of the need to manage depreacations in absence of symfony-bridge. It was actually done there but later removed in https://git.drupalcode.org/project/drupal/-/merge_requests/6326/diffs?co..., after some discussion on Slack.
I'd rather doing it there than in individual tests to have a single point of overriding PHPUnit's own error handler.
- ๐ฌ๐งUnited Kingdom longwave UK
Maybe we should just throw the exception here, but not catch it? PhpStorage is meant to be secure and reliable, if mkdir has failed then we should fail hard instead of trying to ignore problems and carry on.
- Status changed to Needs review
5 months ago 9:46pm 16 April 2024 - ๐ฌ๐งUnited Kingdom longwave UK
The 10.3.x branch says we will throw an exception, the 11.x branch actually throws it. This seems like the simplest solution here.
To avoid holding this up any longer I think adjusting the exception message should be deferred to a followup if needed at all.
- ๐ฌ๐งUnited Kingdom longwave UK
Alternatively we could just drop the warning here?
createDirectory()
returns false in the case that the directory was not created, either the caller can check this, or they can ignore it and carry on and if anything tries to write to the non-existent directory it will fail later down the line.Note that we don't trigger the error in the case the directory exists but we can't change permissions on it (which is sort of what the warning currently implies!)
if (is_dir($directory)) { // Avoid writing permissions if possible. if (fileperms($directory) !== $mode) { return chmod($directory, $mode); } return TRUE; }
- Status changed to RTBC
5 months ago 3:26pm 24 April 2024 - ๐บ๐ธUnited States smustgrave
javascript error on 11.x appears to be unrelated. With 11.x and 10.3 approaching going to go ahead and mark this one.
- Status changed to Needs work
5 months ago 6:00pm 24 April 2024 - ๐ฌ๐งUnited Kingdom catch
I think we should do #23 - drop the warning and let the caller deal with FALSE.
- Status changed to Needs review
4 months ago 4:38pm 26 April 2024 - ๐ฌ๐งUnited Kingdom longwave UK
Implemented #23, we return FALSE on error and propagate this up to the caller instead; adjusted the existing callers and added test coverage.
- ๐ฌ๐งUnited Kingdom catch
I think we might want a follow-up for #15. If there's a single error writing to a file here, then the logging probably isn't that useful, but do we want a hook_requirements() that checks the php storage is writable or something?
Or I guess - what happens to Twig now if the php directory isn't writable? As long as there's some kind of error, then there's nothing else to do here, but if it silently fails, then we need something.
- ๐ฌ๐งUnited Kingdom longwave UK
TwigPhpStorageCache doesn't check the return code of MTimeProtectedFastFileStorage::save(), so after applying this patch if the directory is not writeable there are no errors.
$this->storage()->save($key, $content); // Save the last mtime. $cid = 'twig:' . $key; $this->cache->set($cid, \Drupal::time()->getRequestTime());
Should we add an exception here if
save()
fails? Having said that at the moment the warning is only triggered if creating the directory fails, not on any other kind of failure if the directory exists. - ๐ฌ๐งUnited Kingdom longwave UK
Opened ๐ Add hook_requirements check that PHP storage is writeable Active as a followup.
- Status changed to Needs work
4 months ago 6:19am 1 May 2024 - ๐ฌ๐งUnited Kingdom catch
I don't think the silent failure is a good idea, there's a reason we added the warning in the first place, I've seen it when deploying sites to new environments and messing up permissions/settings.php somehow.
I think we should do #19 or skip the test and re-enable it in a follow-up. Adding a logger here is overkill for a low level system like this, removing the PHP error just seems like it will damage actual live sites.
- ๐ฌ๐งUnited Kingdom longwave UK
Yep given the back and forth here and the fact file_put_contents() also triggers a warning in this case this seems like the most pragmatic thing to do. Pushed #19 with some minor changes.
- Status changed to Needs review
4 months ago 9:01am 1 May 2024 - ๐ฌ๐งUnited Kingdom longwave UK
longwave โ changed the visibility of the branch 3427173-10.3 to hidden.
- ๐ฌ๐งUnited Kingdom longwave UK
Implemented @mondrake's suggestions and fixed the PHPStan baseline.
- Status changed to RTBC
4 months ago 11:41am 1 May 2024 - ๐ฌ๐งUnited Kingdom longwave UK
As far as I know, there are no deprecations of expectWarning/Error left in the baseline, so I think ๐ [meta] Replace calls to ::expectError*() and ::expectWarning*() Active can be closed after this lands.
- ๐ฌ๐งUnited Kingdom longwave UK
Fixed the title and deleted the change record as we changed approach here.
- Status changed to Fixed
4 months ago 1:54pm 1 May 2024 - ๐ฌ๐งUnited Kingdom catch
Yeah I think this is the best we can do, committed/pushed to 11.x and cherry-picked to 10.4.x and 10.3.x, thanks!
Automatically closed - issue fixed for 2 weeks with no activity.