Cannot add hook_menu-defined CSS with drupal_add_css()

So the problem is we should be able to do this:

function mymodule_menu() {
  $items['mymodule.css'] = array(
    'page callback' => 'mymodule_css',
    'access callback' => TRUE,
  );
  $items['mymodule.js'] = array(
    'page callback' => 'mymodule_js',
    'access callback' => TRUE,
  );
  return $items;
}

function mymodule_css() {
  echo "div#crazy { text-align: center; }";
  exit();
}

function mymodule_js() {
  echo "alert('Hello!');";
  exit();
}

function mymodule_init() {
  drupal_add_css('mymodule.css');
  drupal_add_js('mymodule.js');
}

This works for JavaScript files defined in hook_menu() but not for CSS files since there is a check for file_exists in drupal_get_css. We can use menu_valid_path to check if a path is defined by a menu path. Interesting to note that drupal_get_js does not perform any checks for file_exists. Is this something we should standardize in that function as well? Also moving this to 7.x so we can get it fixed, and backport if needed. Anyway, code is ready for review.

Alternate patch that adds the same logic if (file_exists($file) || menu_valid_path(array('link_path' => $file))) to drupal_add_js.

Re-rolled patch.

I don't believe you bot. Patch still applies to HEAD.

Is menu_valid_path() expensive? If so, this needs more thought.

I'll do some benchmarks to see how it affects performance.

Before patch:

Requests per second:    5.19 [#/sec] (mean)
Time per request:       192.635 [ms] (mean)
Time per request:       192.635 [ms] (mean, across all concurrent requests)
Transfer rate:          70.89 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   186  192  15.4    188     327
Waiting:      172  177  14.8    174     312
Total:        186  192  15.4    188     327

After patch:

Requests per second:    5.11 [#/sec] (mean)
Time per request:       195.661 [ms] (mean)
Time per request:       195.661 [ms] (mean, across all concurrent requests)
Transfer rate:          69.79 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   186  195  23.4    189     477
Waiting:      172  180  22.0    175     437
Total:        186  195  23.4    189     477

This was on the homepage with the following js/css added:

<link type="text/css" rel="stylesheet" media="all" href="/modules/book/book.css?f" />
<link type="text/css" rel="stylesheet" media="all" href="/modules/node/node.css?f" />
<link type="text/css" rel="stylesheet" media="all" href="/modules/poll/poll.css?f" />
<link type="text/css" rel="stylesheet" media="all" href="/modules/system/defaults.css?f" />
<link type="text/css" rel="stylesheet" media="all" href="/modules/system/system.css?f" />
<link type="text/css" rel="stylesheet" media="all" href="/modules/system/system-menus.css?f" />
<link type="text/css" rel="stylesheet" media="all" href="/modules/user/user.css?f" />
<link type="text/css" rel="stylesheet" media="all" href="/modules/forum/forum.css?f" />
<link type="text/css" rel="stylesheet" media="all" href="/themes/garland/style.css?f" />
<link type="text/css" rel="stylesheet" media="print" href="/themes/garland/print.css?f" />

Keep in mind that when CSS/JS aggregation/compression is turned on, this will have minimal impact.

The standard deviation in the second benchmark is too big to be useful. Perhaps try with more requests.

Re-rolled and marking needs benchmarks.

Is menu_valid_path() expensive? If so, this needs more thought.

I have no access to a benchmarking environment right now, but this patch doesn't (or shouldn't) affect performance for any case that currently exists.

file_exists($file) || menu_valid_path($file) is equivalent to file_exists($file) unless you actually do give it an internal system path, and then the performance cost is linear to the number of system paths you give it. This patch doesn't affect performance itself; it only makes it an option for module developers to affect performance.

However, note that menu_valid_path not only queries the router table; it also loads placeholder objects. This can indeed get disproportionally expensive if you include a script on, say, node/%node (for whatever reason).

What's the use case for dynamically creating paths like this?

If I wanted to dynamically add css to a page I'd add it inline which is now an option in drupal_add_css(). This saves a full drupal page load.

I imagine I'm missing something. Can someone explain a use case?

I just realized something else - you can bypass the file_exists check by running it through url($path, array('absolute' => TRUE)). Absolute URLs can be loaded as stylesheets without being validated. The only thing you lose that way, compared to menu_valid_path(), is that Drupal will not check if the current user has access, or the path exists - but loading a stylesheet that returns 404/403 will fail silently anyway, so we save ourselves an expensive check.

So combined with the inline CSS mfer describes, now I'm even less sure whether we need this.

Pending other comments, I am setting this to won't fix.

Rationale:

- No module will ever provide a callback for a JS/CSS file that another module includes. Any time a dynamic JS/CSS file is included, your own module includes the code and provides the callback.
- If you have control over the inclusion of the dynamic JS/CSS URL, you can easily make it an absolute URL as outlined above. This will bypass the check.
- Checking for path validity before including a JS file is nice, but it's not vital. If the callback ends up not working, the browser will hit Drupal with a separate request for nothing. But if we check every callback, then all the working callbacks will be checked twice (once during inclusion and once by the separate request). In a trade-off, better to be fast under normal conditions than in error cases.