Regression: aggregated CSS should use server-relative URLs inside url() calls within the CSS file

Linking to a couple related issues, and clarifying the title. I haven't tried out the patch here but the approach looks nice and simple.

This was fixed in #1961340: CSS aggregation breaks file URL rewriting because it abuses it, but recently regressed in D8.

I actually thought that was the issue that caused the regression in Drupal 8? But either way, my understanding is that this broken in D8 only and that D7 handles it correctly.

This looks good to me. Patch still applies. Given that multiple protocols and hosts may point at a drupal site, I can't think of a good reason why drupal should use absolute URLs inside CSS files out-of-the-box. A module can always use hook_file_url_alter() to do that or anything else to the URLs.

Also going to make this "major" as it resolves some mixed-content warnings on d8 sites available via both HTTP and HTTPS.

+1000

Working on a review.

Confirmed that this is a problem.

Working on test coverage.

The patch is a solution to the problem. Because of this:

function file_create_url($uri) {
  // Allow the URI to be altered, e.g. to serve a file from a CDN or static
  // file server.
  \Drupal::moduleHandler()->alter('file_url', $uri);

  $scheme = \Drupal::service('file_system')->uriScheme($uri);

  if (!$scheme) {
    // Allow for:
    // - root-relative URIs (e.g. /foo.jpg in http://example.com/foo.jpg)
    // - protocol-relative URIs (e.g. //bar.jpg, which is expanded to
    //   http://example.com/bar.jpg by the browser when viewing a page over
    //   HTTP and to https://example.com/bar.jpg when viewing a HTTPS page)
    // Both types of relative URIs are characterized by a leading slash, hence
    // we can use a single check.
    if (Unicode::substr($uri, 0, 1) == '/') {
      return $uri;
    }
    else {
      // If this is not a properly formatted stream, then it is a shipped file.
      // Therefore, return the urlencoded URI with the base URL prepended.
      $options = UrlHelper::parse($uri);
      $path = $GLOBALS['base_url'] . '/' . UrlHelper::encodePath($options['path']);
      …

i.e. this patch makes it so that CssOptimizer creates root-relative URLs instead of a shipped file path. Which means it is handled by the first branch, not the second.

However, note this does go against the original spirit of how file URLs are supposed to be passed around inside Drupal: they're supposed to be relative to Drupal's root on the file system root, not relative to Drupal's base URL. And without a leading slash. i.e. core/assets/vendor/jquery/jquery.js, not /subdirectory/assets/vendor/jquery/jquery.js.

It'd be better to fix the second branch in file_create_url(). That'd fix this everywhere: all shipped files (jQuery, theme CSS, etc) would automatically use relative URLs.

Attached patch keeps the existing approach. The test-only patch is also the interdiff.

In the next patch, I'll change to what is IMO the better solution.

D'oh. Accidentally excluded one crucial change.

These are the patches that should've been attached to #11.

Ignore the patches in #11.

As promised:

In the next patch, I'll change to what is IMO the better solution.

Here you go.

I'd worry this could cause breakage for code expecting an absolute URL (often needed outside the site context, such as rendering an e-mail message). Perhaps file_create_url() should accept an $options array so you could pass in ['absolute' => TRUE] when needed?

I'd worry this could cause breakage for code expecting an absolute URL (often needed outside the site context, such as rendering an e-mail message).

But images in e-mail messages (or RSS feeds) are uploaded, they're not shipped. Hence they are of the form public://cat.jpg, not of the form core/assets/vendor/jquery/jquery.js. Those files will still get absolute URLs: their URLs are generated in the stream wrapper's code, for example \Drupal\Core\StreamWrapper\PublicStream::getExternalUrl().

Perhaps file_create_url() should accept an $options array so you could pass in ['absolute' => TRUE] when needed?

Something like that is indeed what is necessary in the long run. i.e. the problem is that file_create_url() is not aware about the context in which its return value will be used, and it's necessary to know that to return the ideal value. We had similar problems about not knowing the context in which data was going to be used in Twig/text sanitization/rendering. This is just one problem that far fewer people ever deal with, so far fewer eyes are on it.

Now it should pass tests.

Fixed those last few failures.

I'm also working on #1494670: References to CSS, JS, and similar files should be root-relative URLs: avoids mixed content warnings & fewer bytes to send, and part of the problem described in that issue is fixed by this issue/patch.

However, upon reading through that entire issue (and the approach in the patch there), I encountered file_url_transform_relative(), which was introduced in #1494670: References to CSS, JS, and similar files should be root-relative URLs: avoids mixed content warnings & fewer bytes to send by … ironically … me :P That approach seems clearly superior.

Instead of a very small chance of breakage (changing file_create_url() this patch), it has zero chance of breakage (using file_url_transform_relative()). So, let's throw out the very very very painful/frustrating work I did with updating all of these pesky tests.

… but then this issue effectively becomes a strict subset of #1494670: References to CSS, JS, and similar files should be root-relative URLs: avoids mixed content warnings & fewer bytes to send. So, I moved the changes here that were still missing in #1494670's patch there, see #1494670-71: References to CSS, JS, and similar files should be root-relative URLs: avoids mixed content warnings & fewer bytes to send.