Random Test Failure with "failed to open stream" for temporary://.htaccess

Thanks @martin107 for the report!

The fail result is at: https://www.drupal.org/pift-ci-job/339437

Aggregator.Drupal\aggregator\Tests\AggregatorRenderingTest
✗	
ile_save_htaccess
exception: [Warning] Line 364 of core/includes/file.inc:
file_put_contents(temporary://.htaccess): failed to open stream: "Drupal\Core\StreamWrapper\TemporaryStream::stream_open" call failedfile_put_contents('temporary://.htaccess', '# Deny all requests from Apache 2.4+.

  Require all denied


# Deny all requests from Apache 2.0-2.2.

  Deny from all

# Turn off all options we don't need.
Options -Indexes -ExecCGI -Includes -MultiViews

# Set the catch-all handler to prevent scripts from being executed.
SetHandler Drupal_Security_Do_Not_Remove_See_SA_2006_006

  # Override the handler again if we're run later in the evaluation list.
  SetHandler Drupal_Security_Do_Not_Remove_See_SA_2013_003


# If we know how to do it safely, disable the PHP engine entirely.

  php_flag engine off
') (Line: 364)
file_save_htaccess('temporary://', 1) (Line: 317)
file_ensure_htaccess() (Line: 1044)
install_base_system(Array) (Line: 658)
install_run_task(Array, Array) (Line: 536)
install_run_tasks(Array) (Line: 115)
install_drupal(Object, Array) (Line: 577)
Drupal\simpletest\WebTestBase->doInstall() (Line: 549)
Drupal\simpletest\WebTestBase->setUp() (Line: 33)
Drupal\aggregator\Tests\AggregatorTestBase->setUp() (Line: 23)
Drupal\aggregator\Tests\AggregatorRenderingTest->setUp() (Line: 1046)
Drupal\simpletest\TestBase->run(Array) (Line: 723)
simpletest_script_run_one_test('25', 'Drupal\aggregator\Tests\AggregatorRenderingTest') (Line: 59)

This also happened in HEAD recently for a different test: https://www.drupal.org/pift-ci-job/529116 In that case there was also a know failure due to #2806697: Random fail for AlreadyInstalledException.

I also wonder if this might be the same bug as #2806697: Random fail for AlreadyInstalledException, just manifesting differently.

More instances:
https://www.drupal.org/pift-ci-job/447899
https://www.drupal.org/pift-ci-job/375643
https://www.drupal.org/pift-ci-job/294179

All early in the alphabet like #2806697: Random fail for AlreadyInstalledException seemed to be.

@catch, @alexpott, @effulgentsia, @Cottser, and I agreed that this is critical as a random test failure under normal circumstances in HEAD.

So this occurs because during the installer the temporary directory is not yet set to the test specific one. We need to move the setting of this to be inside the installer. Unfortunately there is no other way.

Here's another occurrence https://www.drupal.org/pift-ci-job/571518.

oops should have read my own added documentation - the temp directory expected by the test system in not inside the public files directory but it is just up one level.

For example:

    // Prepare filesystem directory paths.
    $this->publicFilesDirectory = $this->siteDirectory . '/files';
    $this->privateFilesDirectory = $this->siteDirectory . '/private';
    $this->tempFilesDirectory = $this->siteDirectory . '/temp';
    $this->translationFilesDirectory = $this->siteDirectory . '/translations';

I just found some older evidence that this has been around a long time : https://www.drupal.org/node/2656720#comment-10872502

This looks good to me, explanation makes sense. @dawehner agrees.

In discussing with @xjm I realised that we can test that we're not writing an .htaccess to the system temp directory and that a .htaccess is present in the test specific directory.

Here's a patch with tests for both Simpletest and BrowserTestBase. Also on @xjm's suggestion I've improved the comments in \Drupal\Core\Test\FunctionalTestSetupTrait to detail where everything actually occurs.

To test this locally:

• Ensure you do not have a .htaccess in your system temp directory - on my system that's /tmp
• Run a test - for example: \Drupal\FunctionalTests\BrowserTestBaseTest
• Without the patch - check that the .htaccess exists in the system temp
• Apply patch and remove the /tmp/.htaccess
• Run a test - for example: \Drupal\FunctionalTests\BrowserTestBaseTest
• /tmp/.htaccess should not exist

Note that the parent site might create an .htaccess in the system temp folder - this is why the above steps should be done from the CLI and we don't assert that a .htaccess does in exist in that location in the new tests.

As this patch does not apply to 8.2.x we'll need to backport once it lands.

In discussing with @xjm I realised that we can test that we're not writing an .htaccess to the system temp directory

Mh, I don't really see this as part of the added test code ...

re #19 -

Note that the parent site might create an .htaccess in the system temp folder - this is why the above steps should be done from the CLI and we don't assert that a .htaccess does in exist in that location in the new tests.

Fair

Committed to 8.3.x, thanks!

Setting PTBP for 8.2.x.

Here's a patch for 8.2.x - there are a number of differences because of improvements made to the how WebTestBase and BrowserTestBase share code.

Backport looks good to me, just a bit more change necessary, moving to RTBC.

Committed/pushed to 8.2.x, thanks!