Replace calls to :::expectWarning*() in Drupal\Tests\Component\PhpStorage\FileStorageTest

Left a comment.

@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?

+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.

Grand. Let's have the two MRs, then. On it.

+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.

Fleshed the CR, ready for review

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.

Change looks good. CR reads fine to me.

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.

#15 if it's for developers, we could use an assert()? If we need logging, it's quite hard. This is a component.

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.

I think the best thing here might be to add our own error handler in the test of the warning and be done.

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.

#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.

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.

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.

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;
      }

javascript error on 11.x appears to be unrelated. With 11.x and 10.3 approaching going to go ahead and mark this one.

I think we should do #23 - drop the warning and let the caller deal with FALSE.

Implemented #23, we return FALSE on error and propagate this up to the caller instead; adjusted the existing callers and added test coverage.

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.

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.

Opened 📌 Add hook_requirements check that PHP storage is writeable Active as a followup.

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.

Yeah I think we should do #19. It's not that bad for this one tricky one.

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.

longwave → changed the visibility of the branch 3427173-10.3 to hidden.

Nitpicky suggestion inline.

Implemented @mondrake's suggestions and fixed the PHPStan baseline.

Is this the last issue of this series?

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.

Fixed the title and deleted the change record as we changed approach here.

catch → closed merge request !6998

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!

mondrake → closed merge request !7003

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