Allow Inline CSS in drupal_add_css

Ugly hack/work around, which i'm sure is the "wrong" way to do it, but it works for me.

$color = variable_get('mymodule_color', '#999');
$css = "p {background-color: $color }";

drupal_set_html_head(drupal_get_html_head() . "<style type="text/css">$css</script>");


echo '<p>This text should appear with a greyish background.</p>';

drupal_set_html_head imho has a bad name since it's really literally "Add output to the head tag of the HTML page."

what you want is drupal_set_html_head("<style type="text/css">$css</script>");

Marking for a review sometime this weekend.

subscribe.

This looks to work but, there are no tests against this functionality to test that it works. Can we get a test?

When CSS preprocessing is turned off the added css is inline. When CSS preprocessing is turned on the added CSS is appended to the preprocessed file. Is this the intent?

That shouldn't be the case, no.

Also, what should we test?

this patch seems to work here

there are some wrong spaces see

   if (file_exists($file)) {
     // Load the local CSS stylesheet.
     $contents = file_get_contents($file);
-
+    
     // Change to the current stylesheet's directory.
     $cwd = getcwd();
     chdir(dirname($file));
 
-    // Replaces @import commands with the actual stylesheet content.

i wonders why you wrote "# Added a couple tests for the inline stylesheets" but there is nothing in the patch file

What about letting the "preprocess" option default to TRUE for files and FALSE for inline code?
The reason for it is that "inline" style is probably more often random (and causes too much cache poisoning therefore).
Is that too much black magic?

You appear to use $data for both the outer and inner foreach loop now ("foreach ($types as $type => $data)" and "foreach ($types[$type] as $data => $preprocess)").
Since the first $data does not get used (but $types[$type] instead), it does no harm, but causes confusion and since it's not being used, I suggest changing it to "foreach (array_keys($types) as $type)".

In one of the test it says

t("Rendering preprocessed inline CSS adds it to the page. '$css_preprocessed'")

I don't believe $css_preprocessed should be listed here.

Other than that, it looks good to me.

Subscribe

Looks good, tests pass, manually tested and that passes as well.

--
Matt
www.mattfarina.com
www.innovatingtomorrow.net
www.geeksandgod.com
www.superaveragepodcast.com

+ * @param $data
+ *   (optional) If given, the value depends on the type passed through to the
+ *   $options parameter:

I had to read this sentence 3 times to understand it. Especially, since "type" seems to refer to $options['type'].

+ *     For example a CSS file called 'name.css' will have a 'name-rtl.css'

A few lines above, we are recommending to prefix filenames with the module's name. This example should adhere to that.

+ *   - 'inline': The CSS that should be placed in the given scope.

Can we stuff a "string" into that description to make the data type clear?

+ * @param $data
+ *   - 'module' or 'theme': The path to the CSS file relative to the base_path(),
...
  * @param $options
  *     - 'type'
  *       The type of stylesheet that is being added. Types are: 'module',

@param docs are not only using different indentation, but also a different layout for descriptions.

+ *       when being outputed on the page.

Not sure about the past tense of "output" here.

+      // Don't aggregate inline stylesheets.
+      if ($type != 'inline') {

Please do not use abbreviations in comments. Also, this comment just duplicates what is obvious from reading in the next line. Comments should describe the how's and why's instead.

 /**
+ * Processes the content of a stylesheet for optimization.

First line of PHPDoc blocks should not use the third form. Replace with "Process". "for optimization" is a bit confusing here.

+ * @param $contents
+ *   The content of the stylesheet.

$contents is the content? ;)

Lastly, there should be at least one test that verifies the real output and not a direct call to drupal_get_css().

Sorry, I should have made my point clearer:

Not so good:

+ * @param $data
+ *   (optional) The stylesheet data to be added, depending on what is passed
+ *   through to the $options['type'] parameter:
+ *   - 'module' or 'theme':
+ *       The path to the CSS file relative to the base_path(),
+ *       e.g., /modules/devel/devel.css.

Good:

+ * @param $data
+ *   (optional) The stylesheet data to be added, depending on what is passed
+ *   through to the $options['type'] parameter:
+ *   - 'module' or 'theme': The path to the CSS file relative to the base_path(),
+ *     e.g., /modules/devel/devel.css.

Also, I think we should test the real, expected output, i.e.

'<style type="text/css">' . $css . '</style>'

Oh, and on a second thought, I think we want to throw a notice to

+ *   - 'inline':
+ *       A string of CSS that should be placed in the given scope.

just like we do for inline JavaScript - in general, usage of 'inline' is not recommended.

Dumdedumdedum....

+ *   - 'module' or 'theme': The path to the CSS file relative to the base_path(),
+ *     e.g., /modules/devel/devel.css.

"The path to the CSS file relative to the base_path()" - but "/modules/devel/devel.css" is _absolute_. :P

+ *   - 'inline': A string of CSS that should be placed in the given scope. Note
+ *     that it is better practice to use 'module' or 'theme' stylesheets, rather
+ *     then 'inline' as the CSS would then be aggregated and cached.

then => than

  * @param $options
  *   (optional) A string defining the type of CSS that is being added in the

Can we stuff single quotes around type here?

+        // Include inline stylesheets

Missing punctuation.

+ * @return
+ *   Contents of the stylesheet including the imported stylesheets.

"including any imported stylesheets via @import" (or similar) would make this a bit more grokable on first sight.

Afterwards, RTBC.

Sorry, just ...

  * @param $optimize
- *   Defines if CSS contents should be compressed or not.
+ *   (optional) Defines if CSS contents should be compressed or not.
...
 function drupal_load_stylesheet($file, $optimize = NULL) {
...
+ * Process the content of a stylesheet for aggregation.
...
+ * @param $optimize
+ *   (optional) Defines if CSS contents should be compressed or not.
...
+function drupal_load_stylesheet_content($contents, $optimize = FALSE) {  

"compressed" should probably be replaced by "aggregated". However, after reading the code, it seems like $optimize is neither of both, but the CSS is "minified" instead. Proposal:

 * @param $optimize
 *   (optional) Boolean whether CSS contents should be minified.

Originally, I wanted to append "Defaults to FALSE.", but the recursive logic does not allow that...

Reading even more code, I would even say that for both functions, in reality, $optimize is not optional at all. It was just made optional to allow for the recursive calls (for imported stylesheets), but when directly called from elsewhere, the argument should always be passed?

/me grins

Unfortunately, the second instance is not yet updated:

+ * @param $optimize
+ *   (optional) Defines if CSS contents should be compressed or not.

Hrm. Basically, this would also make this obsolete:

  // Store optimization parameter for preg_replace_callback with nested @import loops.
  if (isset($optimize)) {
    $_optimize = $optimize;
  }

However, I am not entirely sure whether we are overlooking a part of the logic that may was the reason for using NULL instead of FALSE by default. I grepped over core, but could not find a function that (is not in common.inc) and invokes drupal_load_stylesheet() without the second argument.

The only other is _drupal_load_stylesheet(). So I have to wonder whether it was implemented this way, because drupal_add_css() processed all CSS in a certain order, invoking drupal_load_stylesheet() subsequently, and (perhaps?) relying on a previously defined $optimize state?

Hint: This is what inline comments should really describe. "Store optimization parameter for preg_replace_callback with nested @import loops." explains nothing.

ok, let's revert anything that has to do with the default value of $optimize (but keeping the "minified" term improvement). I'll do the final review.

Thanks!

This is ready to hit the core.

I really hope you have thought about #228818: IE: Stylesheets ignored after 31 link/style tags... this is OT here, but as more style tags we add to a page - the sooner we are running into this critical issue.

Excellent. THX.

Subscribe - great work guys :)

+   * Tests rendering inline stylesheets with preprocessing on.
...
+   * Tests rendering the stylesheets with preprocessing off.
...
+   * Tests rendering the inline stylesheets through a full page request.

Can we make this a bit more consistent while core maintainers are unavailable?

Otherwise, this looks really, really RTBCRTBC.

subscribe

Great work, folks! Committed to HEAD with some comment standardization, per sun.

Marking needs work until this is in the upgrade documentation.

For those who are interested, here is a patch against 6.x-dev, adaptated from #49. I tagged the issue as "need review" but I don't really hope it will be back-ported.

Thank you B-Prod for your patch. I just added the \n to line 148.

#61: drupal-core-add-inline-css-259368-61.patch queued for re-testing.