JavaScript not themable
mfer - September 14, 2009 - 13:40
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | theme system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed |
Description
The JavaScript for drupal is generated by drupal_get_js. Not one does it do the preprocessing magic it renders the xhtml output. Since this output is not sent through a theme function it cannot be overriden. The current output is valid xhtml. But, as html5 becomes more popular some will want to change the JavaScript output to be valid html5.
A simple example of the problem....
Currently we have something like:
<script type="text/javascript">
<!--//--><![CDATA[//><!--
jQuery.extend(Drupal.settings, { "basePath": "/" });
//--><!]]>
</script>This is valid xhtml. We actually need the CDATA for the xhtml to validate. This is not the case with html5. Valid html5 markup would look like:
<script>
jQuery.extend(Drupal.settings, { "basePath": "/" });
</script>The default type for script is text/javascript so we don't even need to specify that (though we could).
The inability to change the markup is a bug we need to fix. Patch to come soon.
#1
Oh dear god yes.
#2
This is a companion issue to #576940: links tags which reference css are not themable. There is, also, #552478: Improve link/header API and support on node/comment pages rel=canonical and rel=shortlink standards which I am not opposed to. But, with so little time until D7 is actually frozen for API changes I don't see that one going in.
#3
Very nicely done! So close, a couple small changes and it's good.
+++ includes/common.inc 2009-10-07 15:06:28 +0000@@ -3417,12 +3417,6 @@ function drupal_get_js($scope = 'header'
- // For inline Javascript to validate as XHTML, all Javascript containing
- // XHTML needs to be wrapped in CDATA. To make that backwards compatible
- // with HTML 4, we need to comment out the CDATA-tag.
- $embed_prefix = "\n<!--//--><![CDATA[//><!--\n";
- $embed_suffix = "\n//--><!]]>\n";
I like this change a lot!
+++ includes/common.inc 2009-10-07 15:06:28 +0000
@@ -3459,7 +3456,7 @@ function drupal_get_js($scope = 'header'
// starting with "ad*".
$filename = 'js_' . md5(serialize($files) . $query_string) . '.js';
$preprocess_file = file_create_url(drupal_build_js_cache($files, $filename));
- $preprocessed .= '<script type="text/javascript" src="' . $preprocess_file . '"></script>' . "\n";
+ $preprocessed .= theme('script', NULL, array('src' => $preprocess_file)) . "\n";
}
Having a NULL argument first looks kind of weird, but it makes sense, since its the contents of the script, and this script tag isn't meant to have anything in it.
+++ includes/theme.inc 2009-10-07 14:57:31 +0000@@ -2019,6 +2019,33 @@ function theme_indentation($size = 1) {
+
+ //set a default value for type
+ if(!isset($attributes['type'])){
+ $attributes['type'] = 'text/javascript';
+ }
"// Set a default value for the type." ;-)
+++ modules/system/system.module 2009-10-07 15:02:38 +0000@@ -1664,7 +1664,7 @@ function system_admin_menu_block($item)
SELECT m.load_functions, m.to_arg_functions, m.access_callback, m.access_arguments, m.page_callback, m.page_arguments, m.title, m.title_callback, m.title_arguments, m.theme_callback, m.theme_arguments, m.type, m.description, m.path, m.weight as router_weight, ml.*
- FROM {menu_router} m
+ FROM {menu_router} m
LEFT JOIN {menu_links} ml ON m.path = ml.router_path
WHERE (ml.plid = :plid AND ml.menu_name = :name AND hidden = 0) OR (m.tab_parent = :path AND m.type IN (:local_task, :default_task))", array(':plid' => $item['mlid'], ':name' => $item['menu_name'], ':path' => $item['path'], ':local_task' => MENU_LOCAL_TASK, ':default_task' => MENU_DEFAULT_LOCAL_TASK), array('fetch' => PDO::FETCH_ASSOC));
foreach ($result as $link) {
@@ -1698,7 +1698,7 @@ function system_admin_menu_block($item)
@@ -1698,7 +1698,7 @@ function system_admin_menu_block($item)
}
}
if ($has_subitems) {
- // If we've had at least one non-tab subitem, remove the link for the
+ // If we've had at least one non-tab subitem, remove the link for the
These whitespace changes probably shouldn't be in this patch.
I'm on crack. Are you, too?
#4
Here is an update based on the feedback from Rob Loach.
#5
The whitespace changes are minor, and they have to happen anyway. RTBCing!
#6
wow, erm, now you're trying to sneak in that theme function for links in here?
#7
Sure, that's a great API design. theme_css(), theme_script(), theme_link(), theme_........
So much time wasted instead of working on the proper solution.
#8
FYI: I'm referring to #552478: Improve link/header API and support on node/comment pages rel=canonical and rel=shortlink standards (again).
#9
@sun If I thought that issue would get into D7 I would pursue it. But, I find this one to be much more likely. I would rather have this and the css counterpart in core rather than putting all my eggs in the basket of #552478 and have it not go in. This is a simple bug fix which creates a minor change. That is proposing an API change that isn't ready to go yet.
#10
Should the interface to the theme function be a $element array? Like those used in to many other parts of drupal?
The function definition would looke like:
<?phpfunction theme_script(array $element = array()) {
?>
And the input array would look like:
<?php$element = array(
'#value' => 'The inline js goes here',
'#attributes' => array('defer' => 'defer'),
);
?>
This change would mean the script tag can be used by drupal_render as well.
#11
I prefer #552478: Improve link/header API and support on node/comment pages rel=canonical and rel=shortlink standards if we can get it ready.
This should be blowing up massively because all theme functions take a single array now.
#12
Postponing in light of #552478: Improve link/header API and support on node/comment pages rel=canonical and rel=shortlink standards. If that goes in we don't need this one.
#15
Thanks for the patch!
#16
#552478: Improve link/header API and support on node/comment pages rel=canonical and rel=shortlink standards was committed and added theme_html_tag(), which is now used for both the JavaScript and CSS script tags. This is now considered fixed :-) .
#17
Automatically closed -- issue fixed for 2 weeks with no activity.
#18
what is closed?