Support passing/defining LESS variables/functions via drupal_add_css() and theme.info

One broken issue (this patch is too naive) is server-side caching: LESS files are not replaced/updated when a passed PHP variable changes its output. For testing purposes, putting LESS module into Developer Mode bypasses this issue (LESS files are recompiled every time - but that's too wasteful for production).

I'll see about fixing this in a day or two.

Fixed cache problem by rearranging the settings passable to LESS:

$less_args = array(
  'expire' => FALSE, //Setting to TRUE will force expiration of the LESS file, regardless of all else
  'tag' => '', //This will prefix any generated CSS file, thus letting you cache settings on a per-tag basis (e.g., setting to 'subsite1' for all settings changed for Subsite 1 will cache those files separately.
  'functions' => array(), //Key => value array where key = LESS function (used in .less file) and value = the PHP function callback.  Be careful while registering PHP functions as it is a security risk!
  'variables' => array(), //Key => value array where key = name of LESS variable (sans @).
);

Example:

function lessphp_double($arg) {
    list($type, $value) = $arg;
    return array($type, $value*2);
}
$less_args = array(
  'tag' => '',
  'variables' => array(
    'color-base' => '#330000',
  ),
  'functions' => array('double' => 'lessphp_double'),
);
if ($in_subsite1) {
  $less_args['tag'] = 'subsite1'; //Invoke other LESS file cache
  $less_args['variables']['color-base'] = '#003333';
}
drupal_add_css(
  $theme_path . '/css/styles.css.less',
  array(
    'group' => CSS_THEME,
    'preprocess' => false,
    /* This is the magic: */
    'less' => $less_args,
  )
);

Oops... I messed up the order of adding $less_args['tag'] to the filename in the patch. Hand-rolling patches is dangerous. This should work.

Oh my! Can you give me a specific piece of the patch that fails so I can fix it? After glancing over them, I think my "invalidation" (not true invalidation) works with the patches in #1347258: Race condition (because of how it is accomplished - it does not use the real flushing mechanism). I've tested the patch in 7.x-2.5 with LESS Devel and CSS aggregation on and can't recreate the race condition problem.

Basically, the tag system is more like dynamically adding another .less file to the stack than invalidating the cache. The expire = TRUE method just adds an OR condition to the file_exists() check - it all happens after the _less_new_dir() call truly invalidates the cache:

function _less_pre_render($styles) {

// ...

// Cache invalidation/flush:
if ($less_devel || !$less_dir) {
    $less_dir = _less_new_dir();

// ...

// Here is the tag-based invalidation (not really invalidating prior caches
// - this is intentional since people may navigate back to the place where the previous
// settings were, so the theme/module that is changing settings needs to manage
// its own "tag" set, and only when things actually change, which makes sense to me.
// I thought about MD5-ing the $settings but decided it was too expensive to do that
// for each file on every access.  Instead, I'm leaving the decision about whether
// a change in $settings changes the output to the theme developer, who probably
// knows their .less files better than I do.)
      $output_file = $css_path . '/' . $less_args['tag'].basename($input_file, '.less');

// ...

// Here is expire=TRUE:
      if (!file_exists($output_file) || $less_args['expire']) {
        require_once($lessphp_include);

// ...

Either way, will the other method you mention allow creating reusable (and cached client-side) sets of settings and function registration, as well as variable passing? The reusable settings sets is particularly important for mobile devices. This patch's method lets me optionally send as few as one changed LESS file (technically a differently-named one), and the rest (which I don't set a "tag" for) will remain unchanged in the mobile browser's cache. When the user navigates to previously viewed areas, that same single file (with its unique file name and set of settings) is still cached, saving a lot of network traffic and time.

For my subsite example, the majority of the CSS is bundled and unchanging, but one .less file changes some border/banner colors dynamically depending on the URL. I want to cache it on the client-side so changing the URL-dependent colors alone doesn't cause everything else to be redownloaded, and when the visitor returns to previously visited pages the color settings also hit a primed cache.

Cache example w/changing "tag":

1. Base site visit

css/big-aggregated-LESS-stuff.css
css/small-single-color-changing-stuff.css (preprocess=FALSE, so it's not aggregated)

2. On Subsite 1

css/big-aggregated-LESS-stuff.css (still resident in server- and client-side cache from previous page)
css/subsite1-small-single-color-changing-stuff.css (just the changed stuff is marked with a new tag and needs to be downloaded)

3. Return to base site

css/big-aggregated-LESS-stuff.css (still resident in server- and client-side cache)
css/small-single-color-changing-stuff.css (still resident in server- and client-side cache)

Ah! Thanks for the clarification! And it makes total sense, so I thought maybe we needed to abandon $expire=TRUE (which is okay because I don't use it anyway.) $tags would still work since the filenames passed to drupal_build_css_cache() would be different, and that's sufficient. Well, I just tried it, and it worked for both! I was surprised, so I did some code excavation and here's why $expire=TRUE also works:

includes/common.inc, lines 3395-3404 (esp. 3399 & 3404):

function drupal_build_css_cache($css) {
  $data = '';
  $uri = '';
  $map = variable_get('drupal_css_cache_files', array());
  $key = hash('sha256', serialize($css));
  if (isset($map[$key])) {
    $uri = $map[$key];
  }

  if (empty($uri) || !file_exists($uri)) {

As you said, the aggregation is based on the SHA256 of the serialized CSS data, but one clarification (and the reason it works) is that it's not just CSS file names in $css; $css includes the $less_args as well. That has important ramifications, because it makes everything work ($tags, $expire=TRUE, changed $variables, etc.)!

So, in my case, just changing expire=1 resulted in these two SHA256 sums (and thus a rebuilt aggregate file!):
93cc0add9efd0b473377fa0283c0129ebbf8aaf1d0d3697ea761679784ae010d
eda0b83bc05543e232a387f6eee1dc36a2544d49a150a3279c86e699fad6d378

Since file_exists($hash) wasn't true, the aggregated CSS file was rebuilt. Obviously it won't rebuild the aggregated file the next time if only expire=TRUE and nothing else is actually changed, though maybe _less_pre_render() can set a dummy $css value to microtime() to force an aggregate rebuild if $expire=TRUE... but that's not critical because it will rebuild the cache when any $less_args['variables'] (or functions, or tags) are changed, which is all that really matters. One thing that could take more thought: if $less_args['variables'] changes, but no new $tag is set and no $expire=TRUE is passed, LESS won't regenerate the file, but Drupal will waste clock cycles aggregating a new set of CSS files. That could be improved, and I'll work on it, time permitting.

Just went through latest patch in #1433948: Add support for theme settings and custom PHP functions and it looks compatible with my patch out of the box. Just need to merge $theme_less w/$less_args['variables'] and $registerFunction w/$less_args['functions'] (the module_invoke_all() for that isn't currently cached, but should be - right now it's called for every .less file) per-.less file.

Doing the above (~2 lines of code), dropping lines 46-52, and copying the rest of the #1433948: Add support for theme settings and custom PHP functions patch should merge both completely. I'll do it tomorrow.

The module invoke shouldn't be called on every .less file; its results don't change, so calling it once before the foreach loop would save some CPU cycles.

Here's a patch that allows you to pass everything via drupal_add_css() and #1433948: Add support for theme settings and custom PHP functions's methods as well.

Both methods now work in this patch; please consolidate #1433948: Add support for theme settings and custom PHP functions into this thread.

To sum up, here's what now works:

1. from issue #1433948: Add support for theme settings and custom PHP functions
   1. passing LESS variables from PHP via theme.info file settings
   2. mapping LESS functions to PHP callbacks via hook_less_register_function
2. from this original issue
   1. passing LESS variables to PHP dynamically at run-time via calls to drupal_add_css()
   2. mapping LESS functions to PHP callbacks dynamically at run-time via calls to drupal_add_css()
   3. server- and client-side LESS cache management
      • package up combinations of certain LESS variables/functions as "tags," so that one actual .less file can create multiple rendered .css files on the server - this lets you change a variable passed to the .less file for certain areas of your site, and reuse those generated files (saves server's CPU time and speeds up the connection for mobile clients, among other bonuses)
      • "expire" a specific .less file to force LESS to regenerate it without flushing the entire LESS cache (also saves the server's CPU time)
      • it must be noted that some kind of cache management is required for all of the functionality to work. Otherwise, even if you change LESS variables or functions from PHP, the new file will not be generated by LESS until the LESS cache is flushed by a site admin. That is why the $tag and $expire=TRUE stuff was added to my patch. The server- and client-side caching bonuses of this approach are just icing on the cake - you'd need some way to force LESS and Drupal to regenerate their files either way. The $tag/$expire=TRUE approach works and as a bonus gets some added benefit along the way (saved CPU cycles, saved network bandwidth, faster mobile browsing) - but I'm not wed to this solution; if there is any better way, let's use it!

Oops, change line 15 of the patch:

-      $global_less_vars += (array) theme_get_setting('less', $theme);
+      $global_less_vars = (array) theme_get_setting('less', $theme) + $global_less_vars;

Original version would have let .info settings override new settings defined in Appearance, so changes would never show up.

If you have a file that has 'expire' set to true (regardless of the above setting) you will be overwriting and existing file. You are leaving the decision to themers to break the race condition fix.

I'm confused - as described in #6, I don't see the problem here since $expire==TRUE is tested after the (possible!) _less_new_dir() call kills the LESS cache.

The 'tag' mechanism is designed to allow you to have a different compilation of a file per arbitrary reason (per page in example), because there might be passed in settings from drupal_add_css(). But this only works if expire is set to true, otherwise you might have settings that are different than what this tagged file was previously generated with and the user is going to expect something different than the old file.

I'd argue that this isn't an issue, and here's why: I'd consider this a corner case, since it only happens when the theme had previously used a $tag (let's say, "subsite1") for one set of $variables (let's say, "@bgcolor=red"), then on another page the theme used the same $tag ("subsite1") but a different set of $variables (let's say, "@bgcolor=blue") without setting $expire=TRUE. Then, yes, the previously rendered subsite1 CSS file would have bgcolor=red since the changed color (blue) was not re-rendered.

That is the theme developer's error and shouldn't be our responsibility to trap for. It's not really different than a developer adding a new theme hook definition and not clearing Drupal's cache, which will obviously break as well. In both cases, it's up to the theme developer to get it right. The developer needs to decide when changes to $variables necessitate invalidating previous files, either with a new $tag or by setting $expire=TRUE. We shouldn't try to guess the developer's intentions - as with other Drupal systems, the onus is on the developer to decide what is required. And that's a good solution because the developer actually knows their desires better than we do.

The utility of $tag is that it preserves caches - both server-side and client-side - in a controllable and predictable fashion. subsite1's settings will remain server-side (and won't waste cycles getting re-rendered) and will remain client-side, saving network traffic. Different, but reusable filenames also mean the system will work with CDNs, mobile Internet proxies, and other common mid-transport caching mechanisms. $tags make all of that work, at the discretion of the theme developer (who again, theoretically knows their intent better than we do).

In the end though, I'll admit the automatic hash/expiration is a good solution as well, and actually I had considered it before $tag. The reason I went with $tag was to avoid more SHA256/MD5 calls which, depending on your virtualization software (and CPUs), may not be hardware-accelerated. Either way, hashing the array requires serialize() which is also somewhat CPU expensive. But maybe I'm trying to eke out every drop of performance and I'm over-engineering for the problem in the process :)

Oh and one more thing, I think LESS variables defined in the theme should only apply to stylesheet files where the 'type' is 'theme'.

I'd agree. I didn't change much of the patch from #1433948: Add support for theme settings and custom PHP functions, except to reduce module_invoke_all() to 1 call instead of looping it in the foreach. I just wanted to bring all the conversation of these enhancements under one issue thread, since they seemed so similar.

How about this compromise?

if (isset($info['less'])) {
        $info['less'] = (array) $info['less'] + $less_args;
        $less_args['expire'] = (bool) $info['less']['expire'];
        // Could do some sanitizing here since 'tag' hits the filesystem, but
        // that is unlike any other module.  If a bad module/theme coder wants
        // to hurt the filesystem, no check here will prevent them anyway;
        // The onus is thus on any implementer to prevent bad user input from
        // getting this far.
        $less_args['tag'] = trim($info['less']['tag']);
        $less_args['functions'] = (array) $info['less']['functions'];
        $less_args['variables'] = (array) $info['less']['variables'];
      }
+      // If variables are passed without a tag, automatically generate one
+      // (this lets conscientious developers manage their own cache, or we'll
+      // cover for them so it works out either way):
+      if ($less_args['tag'] == '' && !empty($less_args['variables'])) {
+        $less_args['tag'] = hash('sha256', serialize($less_args['variables']));
+      }

Also, you still have not addressed that 'expire'==TRUE tells LESS to replace an already existing file, which can break other threads aggregation.

I believe order of operations prevents this from being a race condition issue. $expire==TRUE is checked well after any _less_new_dir() call kills the LESS cache, and before drupal_build_css_cache(). To my understanding, the file operations are protected by the operating system against simultaneous read/write except under a deprecated IIS configuration using ISAPI instead of FastCGI. Apache and IIS w/FastCGI (the recommended PHP configuration) should be safe. If you're running under a non-threadsafe configuration, expect more than LESS to break - you'll get cURL memory corruption errors and problems when Image Styles are rebuilt too, among other issues.

Still, I care more about moving this forward than defending $expire=TRUE, so I've removed it.

Having the hash makes it extremely simple and takes the decision to rebuild out of the hand of the implementer.

Taking the decision away from the implementer is not necessarily good. Helping them in a flexible way is good. The compromise in #16 (where $tag defaults to the hash but can be overridden) is a best-of-breed solution specifically because the implementer has a choice. I can imagine users saying "I'm a smart developer, let me choose when to use a new settings package and save my server some clock cycles." I certainly want to be able to do that.

Additionally, the hash-only solution creates a new bug: it prevents $functions from being updated. If a developer changes a passed $function or the output from that function changes programmatically, there is no way for the developer to inform LESS to rebuild the output. With $tags, there is. So, an informed developer can write better code and has more control; isn't that ideal?

I don't think we should try to guard against all possible errors on the part of the programmer, while making the system less flexible (i.e. more brittle). The entire argument so far against $tags is that a $tag can be reused and the developer won't see their CSS changes until the cache is flushed - they'll be confused by these unexpected results. That is fundamentally no different than if a developer adds some new theme_preprocess() declaration (or any other hook, or any new .tpl.php!) and doesn't remember to clear Drupal's cache; Drupal is okay with this situation, so why aren't we? I think it's fair to expect a developer writing PHP code to understand what they are doing a little. There's no way to prevent all cases of a developer being forgetful; in the pursuit of an unattainable goal, we shouldn't remove useful functionality and force a bug (no way to alert LESS that a function has changed).

For most developers, $tag will automatically use the hash. For developers who know what they are doing and need a little more flexibility, they can have more control.

Here's a patch without $expires, but with the best-of-breed, everyone-can-be-happy, basic-implementers-won't-fail-but-advanced-developers-have-a-choice compromise. I've also fixed the theme.info settings getting passed to all .less files; now it'll only impact group=CSS_THEME files.

If your function is changing its process based on data other than what is passed in, then chances are you're doing something wrong.

A function may change process based on some Drupal setting. I won't presume to guess what someone on this great big Internet might want to accomplish, but it shouldn't matter; there's no reason to close off this ability - no strong argument has been made for locking it down. In fact, "if you can't do it our way, you're probably doing something wrong" sounds like the opposite of the argument used against $tags: "we need to make sure that there is absolutely no way for a programmer to get unexpected results."

Why are we okay with unexpected results (which can't be eliminated) in one place, but not another? $tags allows both behaviors, solves the corner case of function process change, and defaults to the helpful behavior you want. The only point against it is that it allows an override for experienced developers (which isn't really a point against it). To me, for all reasons stated so far (esp. the summary in #19), that sounds Drupal-like and desirable. For most developers, $tag will automatically use the hash and the typical use case works. For developers who know what they are doing and need a little more flexibility, they can have more control. That's Drupal.

On the performance front, Drupal hashes the styles list every page when aggregation is enabled, to check if the aggregate file already exists or if it needs to be created.

That's specious; just because Drupal uses an expensive process out of necessity, it does not mean we should also use an expensive process if it can be avoided. Extrapolating that logic would imply every single module ever written can/should feature expensive function calls because some other module has used them previously. It's bad logic. Each module should do its best to be as low-impact as possible.

If the developer gives you nothing better to work with, then do the hash. But there's no reason not to allow an override by experienced developers. What you are arguing for is stripping all control over the process from the developer, in order to protect someone from potentially making a corner-case mistake. As said previously, there's no good justification for this over-zealous protection - the programmer can still make a mistake (it's a battle we can't win), but in this case they are limited from solving other problems. That'd be like designing a car which couldn't exceed the speed limit but can still drive into oncoming traffic; drivers can still make errors, so the situation isn't improved, and now the driver is out of luck if his wife is going into labor.

In sum, the argument against $tags has been "someone might change a setting under a re-used $tag, and then not see the new changes!" I think it's perfectly fine if someone reuses a $tag and doesn't see their changes. They are purposely using the override functionality; they shouldn't do that unless they understand what they are doing. If someone changes an @imported file, they won't see their changes either. Or if they add a .tpl.php or a new hook definition in template.php without clearing Drupal's cache. In general, if developers mess up, they will experience unexpected results. We can't solve that. Why destroy good functionality by trying? 99% of people will never even employ this added capability, but that's all it is - added functionality a developer may use (or may ignore!). It costs us nothing to include it, so how is it a bad thing? The default is still exactly the case you want. It just lets us use our judgment to override the default if we choose.

This looks like a perfect fit for the patch in this thread (or attiks' thread, though that functionality has been rolled into this thread): just pass your callback function in to drupal_add_css() as $parameters['less']['functions']['dehash'] => 'your_php_function_name'; and you're done! No need to access the actual less parser object.

What problem did you have when applying the patch?

Reroll of the patch against latest 7.x-2.x-dev branch.
Updating the issue title to more accurately reflect what the patch attempts to accomplish.