When AssetControllerBase delivers existing file should add content-type

Thank you for reporting

As a bug though we will need a test case showing the issue also.

Attached a modified patch which adds a new assert to Drupal\FunctionalTests\Asset\AssetOptimizationTest::assertAggregate to check response always delivers content-type header.

+++ b/core/tests/Drupal/FunctionalTests/Asset/AssetOptimizationTest.php
@@ -133,6 +133,7 @@ protected function assertAggregate(string $url, bool $from_php = TRUE): void {
     $headers = $session->getResponseHeaders();
+    $this->assertArrayNotHasKey('Content-Type', $headers);
     if ($from_php) {

Shouldn't this be assertArrayHasKey?

Sure. Updated patch is attached.

Adding a test-only patch.

The patch doesn't fail, I think this is because the test doesn't execute the 'from existing file' code path which should only really happen during stampedes.

Probably requires something like this:

We'd need to file_get_contents() the aggregate, call $this->flushCaches() to delete all aggregate files etc., then manually write back the file to it's original location with file_put_contents(), then request the aggregate URL again with the browser. That is equivalent to a stampede/race condition where another process has created the file after the asset request happened but before we check if the file exists.

#8, if I'm reading correctly. Sounds like the tests need updating. Though seems like a lot but that's just me.

I just tried to write a test for #8, and just as it was nearly there, I realised it's impossible.

If we clear the cache then write the file, the file will exist on disk, and then when we request it with the browser, because it exists on disk, the webserver will just serve the file directly bypassing the controller. So it really is only possible to reproduce in the race condition/stampede case and I can't think how to simulate that - i.e. we'd have to request the controller with no file, then before it tries to create the aggregate, create the file underneath it.

The extra assertion added in #6 doesn't do any harm though, it ensures we're getting a content type from the other two cases, so I think that makes #6 RTBC.

This whole issue is a workaround for 📌 Register Drupal's mime guesser with the non legacy Symfomy singleton Needs work because this all ought to be handled automatically, so I've added a @todo pointing to that, but otherwise the patch is identical to #6. Once we fix 📌 Register Drupal's mime guesser with the non legacy Symfomy singleton Needs work and remove the workaround, this shouldn't need explicit test coverage anyway.

Figured out a way.

We can't reproduce the exact race condition, but routes are case insensitive in Drupal - so if we capitalise the directory, then we hit the 'race condition' code path because that will hit PHP and also find the correct file. It is a hack, but I don't see another way to get coverage.

Without the cs error.

The last submitted patch, 12: 3371358-12-test-only.patch, failed testing. View results →

Going to rely on the fail/pass from #12.

+++ b/core/tests/Drupal/FunctionalTests/Asset/AssetOptimizationTest.php
@@ -103,8 +103,15 @@ protected function doTestAggregation(array $settings): void {
+      $this->assertAggregate($url, 'text/css');

I think this is missing the argument name? Second arg is a boolean, so this should likely be content_type: 'text/css'

I think strict types would have caught this

It's a bit early and took me a minute to figure out that 'strict types' meant 'declare(strict_types=1)' and not 'having a type hint'.

RTBC +1 @catch #16 🐛 When AssetControllerBase delivers existing file should add content-type Fixed this resolves the issue for us.

#15 appears to be addressed so remarking.

Re-rolled for fuzz after 📌 Tighten xpath selectors to decrease complexity in tests Fixed

Committed cccf3c6 and pushed to 11.x. Thanks!

Backported to 10.1.x

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