Allow external CSS files through drupal_add_css

subscribe

subscribe

Tagging.

The attached patch allows external CSS to be added via drupal_add_css(). Tests included.

I removed the unset() call from drupal_get_css().

It seems that this patch could go in before the weight one, or after, so long as there is coordination.

1.  * @param $options
    *   (optional) A string defining the 'type' of CSS that is being added in the
    *   $data parameter ('module', 'theme' or 'inline'), or an associative array of
    *   additional options, with the following keys:
   

   This section should also include the new "external" keyword

2. From the Doxygen comments at the start of drupal_add_css:

    *     CSS preprocessing is the process of aggregating a bunch of separate CSS
    *     files into one file that is then compressed by removing all extraneous
    *     white space. Note that preprocessed inline stylesheets will not be
    *     aggregated into this single file, instead it will just be compressed
    *     when being output on the page.
   

   Should we add a little something about the exclusion of external stylesheets during aggregation?

3. + *   - 'external': Inlcude an external CSS file that is not hosted on the local
   

   Typo.

4. +  function testAddExternal() {
   +    $path = 'http://example.com/style.css';
   +    $css = drupal_add_css($path, 'external');
   +    $this->assertEqual($css['all']['external'][$path], TRUE, t('Adding an external CSS file caches it properly.'));
   +
   +    
   +  }
   

   Some excess whitespace.

5. It does not make sense to add the dummy query string to external stylesheets (to control browser caching). I think we should drop it when the stylesheet is external.
   Example:

   <link type="text/css" rel="stylesheet" media="all" href="http://news.bbc.co.uk/css/screen/1_0_4/nol/v4/index.css?H" />
   

6. Should we validate the link or is this the responsibility of drupal_add_css' caller? For example, this works:

   drupal_add_css('http://example.com/index.css" /><script>alert("help");</script>', 'external');
   

Other than this looks good. All tests pass, and I played around with it on a test installation and it works as expected.

This patch addresses the first 5 issues mentioned by mr.baileys. I've left out validating the link itself. I'd say we either leave this to the calling function, or it belongs in a separate issue (drupal_add_js/drupal_get_js doesn't validate external links either). The alternative, I suppose, would be to pass $media through the url() function.

subscribe

testbot is happy and I'm also happy ;)

+ $path = 'http://example.com/style.css';

Hm. It makes me really scared to commit code like this knowing that test bot is going to run it 70,000 times per hour. Though I see we already committed a string like this to the JS tests. (it was probably me, too :P)

I would prefer to see this be an absolute link to the site running the tests, unless for some reason this will circumvent the logic we're trying to test. This would also allow us to do more substantiative checks than just "Did > 0 bytes get returned"

Also, just a question -- Why is it desirable to not aggregate external CSS?

(RobLoach answered on IRC that basically this is how we treat JS at the moment. In order to cache external files we would need to come up with an expiry mechanism.)

Setting to 'needs review' - testbot was broken.

@webchick

example.com, example.net and example.org are specifically made for this purpose.
it contains only an index.html and a robots.txt, all other pages return 404
See also RFC 2606, Section 3 and 5

http://testing.drupal.org/node/37
http://testing.drupal.org/pifr/file/1/264876_1.patch

trying out a new testing bot that apparently wasn't working

There was an error in the code comments of the previous patch. An extra / was at the end of a code comment.

The CSS solution essentially the same solution that has already been implemented for external js files. To be consistent, I think we should go this route for now.

I don't like the use of url() here. It doesn't actually add anything to the picture. Because 'external' is set to true all it really does is add an extra fragment and query string onto the end if one is provided. In this case they aren't so there is no benefit. url() with external passed in doesn't do any validation in D7. See http://api.drupal.org/api/function/url/7

If we want to check for a valid url we can use something like valid_url (http://api.drupal.org/api/function/valid_url/7) which uses a not so simple (or fast) regex or we can use something like the PHP filters which doesn't validate to spec. So, it will tell you it's roughly the right format but it still may not be.

url validation is hard. valid_url is to spec (RFC 3986) but the spec doesn't reflect reality. For example, the spec doesn't allow for international characters in urls. Well, you can go to the site http://☃.net/ (it routes) or http://例え.テスト/メインページ. There is an issue for this already.

For the time being I would remove the url() call and expect the caller to make sure it's valid.

The attached patch removes the extra / and the calls the url().

This patch is great, since it is the final piece (I think?) that gives us symmetry between drupal_add_js() and drupal_add_css().

But, let's complete the symmetry:

+ *   - 'external': Include an external CSS file that is not hosted on the local
+ *     server. These files will not be aggregated if CSS aggregation is
+ *     enabled.

in drupal_add_js() this is:

 'external': The absolute path to a JavaScript file hosted externally. 

Can we make the descriptions of both of these a hybrid of the two that explains the parameter is asking for an absolute path, but also notes that files put in this way are not aggregated? (Unless they are in JS and not in CSS for some reason, and if so it'd be good to point that out specifically, too.)

+      default:
+        // Stylesheets of type "file" and "external" both use the location as the
+        // array item's key.
+        $css[$data] = $options;
+        break;

in drupal_add_js():

      default: // 'file' and 'external'
        // Local and external files must keep their name as the associative key
        // so the same JavaScript file is not be added twice.
        $javascript[$options['data']] = $options;
        break;

In this case, I think drupal_add_js()'s description is actually more helpful, so let's duplicate it here.

Also, no need to break; on a default.

+      case 'external':
+        // Preprocessing for external JavaScript files is ignored.
+        $external_css .= '<link type="text/css" rel="stylesheet" media="' . $item['media'] . '" href="' . $item['data'] . '" />' . "\n";
+        break;

Too much symmetry. ;) I think you mean "CSS files." ;)

   /**
+   * Tests adding an external stylesheet.
+   */
+  function testAddExternal() {
+    $path = 'http://example.com/style.css';
+    $css = drupal_add_css($path, 'external');
+    $this->assertEqual($css[$path]['type'], 'external', t('Adding an external CSS file caches it properly.'));
+  }
+

I don't see a corresponding test in the JS stuff. Can we please add one?

+  function testRenderExternal() {
+    $css = 'http://example.com/style.css';
+    drupal_add_css($css, 'external');
+    $this->assertTrue(strpos(drupal_get_css(), 'href="' . $css) > 0, t('Rendering an external CSS file.'));
+  }

I realize you're probably trying to save a variable when you don't need one, but everywhere else we do:

    $css = drupal_get_css();
    $this->assertSomething(strpos($css, blahblah));

So let's do the same here.

After that, this looks good to go.

Updated per webchicks comments. The only difference is that instead of $css = drupal_get_css() I used $styles. In many of the test $css is used for the css put into drupal_add_css. I made $styles usage consistent on all CSS tests.

RTBC confirmed, next time, please put on need review first.

Huh. I totally thought I'd committed this already!

Committed to HEAD. Thanks!

Wow I must be tired. :P This needs docs of course.

Docs added http://drupal.org/update/modules/6/7#add-css-external

I don't think the Theme update guide needs it, since themes specify CSS and JS files via .info