links tags which reference css are not themable

Subscribination.

Good point. Subscribe.

superscribe

This patch works by doing the following:

drupal_get_css (in common.inc) was altered to create a series of arrays containing css_items (one for preprocessed, one for non-preprocessed, one for external and one for inline). These arrays (except inline) are merged and sent into a theme_css function (in theme.inc) and then the inline array is sent in separately.

So far the only difference I see when using this patch to the current output is that ALL
tags have a query string at the end, even those that were processed by Drupal and named using an MD5 hash. I realize that the cache-busting query string is unnecessary (because it is part of the hash) but I also don't believe it hurts in any way. Anyone?

Other than that, this is only my 3rd patch (and the first of any substance) so please let me know if I should be doing anything differently

ok .. so my patch-making abilities leave something to be desired ... I think this will work

resubmitting patch because I think it needs a new name .. and I just read about patch naming conventions

gogo simpletest

Unfortunately, I see some tab characters ;-) . Switched to double spaces!

@mfer ... A fair question. I would argue that the theme function does little more than loop through an array isn't any more difficult than the theme_links function. Maybe I'm wrong... anyone else want to weigh in? In any event, I'll give your suggestion a shot and we'll see which is ultimately a better a solution.

This may solve the problem of unnecessarily including a query string after the aggregated style sheets too...

Check back later today

Here is a patch that uses the technique suggested by mfer... After going through it I think this might be the better solution. Any thoughts?

Much nicer. Got another idea that I'd like some thoughts on: What if we passed the entire CSS array through a theme_preprocess_css() function, similar to hook_css_alter(), but then themes could completely swap out CSS elements.

We want hook_css_alter() instead.

I think that this patch and hook_css_alter are mutually exclusive. Why set this to "wont fix"? The original post points out that with the impending arrival of HTML5 to the main stream themers will want to alter the way CSS files are included - meaning the actual markup - in order to be compliant with HTML5 standards.

While I agree that a hook_css_alter function would is an idea worth looking into I imagine that it would be used by themers to remove a CSS item from the CSS array or for adding an additional one. Once the array is altered then the theme function should still be used to actually display the markup.

or am I missing something?

As a side note, at what point does the status of this ticket change to "reviewed & tested by the community" assuming all is well? Like I said, I haven't created too many patches and still feeling out the way things work

I'm resubmitting the patch from #12 because I forgot to include the comments for the two theme functions. This patch has the appropriate comments. Functionally there is no difference.

Because you are introducing a major WTF for developers and themers.

You can completely alter the CSS files for a page already. A theme function would add another layer of indirection to the system, which already has the capability to be altered.

@see http://api.drupal.org/api/function/hook_css_alter/7

This issue is not about changing the CSS files. Its about changing the markup that includes a CSS file.

Lets say a themer decides they want to create a drupal site using HTML4.01... currently they have no way of writing standards compliant markup for the CSS file includes. Something like this (note the ending slash):

  <link type="text/css" rel="stylesheet" media="print" href="/files/css/css_131f0cf18029564f8d026691328015b7.css" >

instead of

  <link type="text/css" rel="stylesheet" media="print" href="/files/css/css_131f0cf18029564f8d026691328015b7.css" />

You can completely alter the CSS files for a page already. A theme function would add another layer of indirection to the system, which already has the capability to be altered.

Can you please explain how the markup above can be altered currently... if there is already a way then that's terrific and you are right that there is no need for this patch. But if not (a) there should be and (b) I dont see how this adds any more complexity - most users will never bother with this theme function but those who are trying to write standard compliant markup really need it.

anyone else wanna chime in here?

I'll point out that you can still put the trailing slash into the link element in HTML5 and it's perfectly valid. It's not that it's not allowed (as is commonly believed), it's just that it's optional (see http://www.w3.org/QA/2008/11/html5-howto.html#c169018)

So, in that respect, this change isn't absolutely necessary, as markup will still validate without it. However, if HTML5 gives you the option, then IMO Drupal should too.

Marking needs review to try and continue the discussion.

Related: #591794: Allow themes to alter forms and page ... It exposes [theme]_css_alter so that the theme can alter the raw CSS array before it's rendered.

@Rob Loach ... can you please explain how altering the raw CSS array will allow themers to decide what markup to use for their
tags? (Like the markup example I put in #18). You are the second or third person who has pointed to this as a solution and I feel like I'm missing something. Can you give me a bit of pseudo code or something to show how I could change the markup with this technique.

perhaps clearer

Sorry, bleen18. I was thinking the CSS that was being passed to the theme. The markup has to be themed as well, thanks.

Apparently, bleen18 did not transfer the knowledge he gained in IRC into this issue.

See #552478: Restrict "self-closing" tags to only void elements in drupal_pre_render_html_tag

I'll let you mark this issue won't fix or duplicate yourself.

mfer ... Ill look into sending an array (ex. ('media'=>'all','href'=>'/path/to/css'...) as the function arguments. That should be more flexible, yes?

If I do that, I could combine the two theme functions into one theme function with something like this:

if($arg['inline'] == true){ ... }else{ ... }

OR do you think its better to keep this as two separate functions (one for inline and one for not-inline)? Two functions may be easier for themers while combining them into one function makes things more concise? Opinions?

Ok @mfer... I liked your idea of simply creating two function theme_link & theme_style though I am a bit worried that the function name "theme_link" may confuse people (I feel like some themers will assume this is used for creating anchor tags...). As for your question about drupal_render, I'm not sure that it's really necessary here. I would equate these two functions to theme_table and theme_links and think they should be handled similarly.

I'm pretty comfortable with this solution... Other thoughts?

I see that this patch has caused "3 exceptions" when the "Cascading style sheets" test was run... but is there a way to get more info about what exactly is failing? This is the first time I've had a patch fail like this.

lets see if this works ... I created my patch with todays dev snapshot instead of the one from tuesday

with a big hand from kwinters (on IRC) I finally tracked down the ridiculous problem with my previous patch. This one should be good to go. Please to review ...

Wouldn't call this critical.

would love to get this in before D7 ... anyone able to review?

It would make much more sense to review #552478: Restrict "self-closing" tags to only void elements in drupal_pre_render_html_tag and provide patches/suggestions on how to make that one also use theme_xhtml_tag() for LINK tags referencing stylesheets. Shouldn't be too hard.

drupal_add_link could also get some theme_link() pimpings! :-)

@mfer - some excellent catches... (still trying to break some old coding habits)

@rob loach - a great suggestion. I included it in this patch

please to look at this patch

oops ... wrong file

ok .. the last patch i put in passes all the tests on my local install of D7. chx has been trying to get command line tests working on my machine but so far we have been unsuccessful. Can someone else try to apply the patch and then run tests via cmd line?

any inkling about whether or not #552478 will go in?

It seems that #552478: Restrict "self-closing" tags to only void elements in drupal_pre_render_html_tag was committed to HEAD, so I believe that mfer is correct and this issue is now fixed (circuitously)