Patch to use JS Aggregation or JS at the bottom of the page
eliosh - July 24, 2008 - 10:34
| Project: | GMap Module |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | critical |
| Assigned: | bdragon |
| Status: | reviewed & tested by the community |
Description
Based on this article :
http://wimleers.com/article/improving-drupals-page-loading-performance
I put all JSes on bottom of page.
I can see that gmap does not work, because module write script tag directly in body.
So i copy and paste theme_gmap function :
<?php
// ...
// $map can be manipulated by reference.
foreach (module_implements('gmap') as $module) {
call_user_func_array($module .'_gmap', array('pre_theme_map', &$map));
}
// Inline settings extend.
$o .= '<script type="text/javascript">'."\n";
$o .= "/* <![CDATA[ */\n";
$o .= 'Drupal.extend({ settings: '. drupal_to_js(array('gmap' => array($element['#map'] => $map))) ." });\n";
$o .= "/* ]]> */\n";
$o .= "</script>\n";
return $o;
?>making this substitution :
<?php
// $map can be manipulated by reference.
foreach (module_implements('gmap') as $module) {
call_user_func_array($module .'_gmap', array('pre_theme_map', &$map));
}
// Inline settings extend.
$o_script .= 'Drupal.extend({ settings: '. drupal_to_js(array('gmap' => array($element['#map'] => $map))) ." });\n";
drupal_add_js($o_script, 'inline');
return $o;
?>I think it'll be great to put this modification in module.
#1
This patch works really well!
As Eliosh says, it is better for performance to load all javascript at the end of the html, so if you want to increase the load performance, just change the page.tpl.php template of your theme in order to load the scripts at the end (moving
<?phpprint $scripts;
?>
<?phpprint $closure;
?>
Also IMHO, it is more of a Drupal standard to load the JS with drupal_add_js function.
Actually that is mandatory if you are working with javascript_aggregator or sf_cache modules.
#2
Please, can someone confirm me that this is a possible patch for future versions ?
I made this patch on a production site and, after upgrading from beta3 to beta5, i spent hours to understand why it doesn't work (without the patch).
Thanks a lot for all your work.
#3
The patch still works great also with 5.x-1.0-rc1!
Works fine with or without JS aggregation module or changing the JS load order in the page.tpl.php
More performance!
#4
Is there any reason why you're not using:
<?phpdrupal_add_js(array('gmap' => array($element['#map'] => $map)), 'setting');
?>
#5
Hi Rob,
I was applying the eliosh recommendation and it helped to understand the change. I agree with you that your code is cleaner.
I have re-rolled the patch removing old lines and leaving only the drupal_add_js. I have tested it and works fine.
Just as a reminder, this change allows that Drupal manage the javascript, allowing that if you are using JS Aggregator to optimize the html and js loading times GMap will work fine.
#6
Much better... This should possibly go into the DRUPAL-6--1 branch if it doesn't already exist as well.
#7
Very cool, thanks.
#8
this is great. thanks for this. it wasn't working for me until this patch. i hope it gets committed
#9
Once again for GMap for Drupal 6.x.
If your theme has the js at the bottom of the page, or you are aggregating JS, you need this to work
#10
I applied the patch, and it worked fine (in 5.x-1.1-rc1). However if I move
$scriptsto the bottom of the page right before$closureinpage.tpl.php, the GMap on the page never loads and I get a "Javascript is required to view this map" message.#11
Confirm Rob patch even form 6.x-1.0
#12
The patch works fine. And it adds the feature to be aggregated to the bottom of a page, increasing load performance. It is also more drupalish than original code.
Is there some issue with that patch that we could help to solve and avoid to being patching every time that GMap module is updated?
#13
I can confirm this works well for 6.x-1.1-rc1 also.
#14
Why this patch is not in included in the module ?
#15
Yup, still not committed.
#16
Works well for me also. I had problems when using with Panels, that randomly the maps were just missing, without any message. Now it works fine.
#17
#18
jcmarco, I have identified the issue. The patch is applied to the
theme_gmapfunction. I changed this ingmap.module, but I had forgotten I was already overriding this theme in mytemplate.phpfile. As soon as I applied the patch intemplate.php, everything works fine.#19
One additional comment: I had attempted to apply this patch suggested by Wim Leers to improve Drupal performance. The patch modifies the
drupal_add_jsfunction incommon.incto ensurejquery.jsanddrupal.jsare loaded at the bottom of the page.When using this patch with GMap, the map is loaded, but no markers are shown. I have looked at the code extensively, and all markers are being generated as expected. However when the map is built on the page, no markers exist (not even a marker "shell" in the HTML).
Any thoughts?
#20
Patch works fine for me. Let's please get this committed.
#21
ron_s, I think that should be a separate issue.
#22
Tried the patch and moved $scripts to the foot of page.tpl.php. Works fine in a Blueprint based theme; although in Admin/Slate part of the map is missing - looks like it is rendering only upper left tile of the map. Tried in FF3 and Saf4 on a Mac.
#23
This patch fixes this issue as well: http://drupal.org/node/266595
#24
I still have this problem.
sites/all/modules/gmap/gmap.module:
// $map can be manipulated by reference.
foreach (module_implements('gmap') as $module) {
call_user_func_array($module .'_gmap', array('pre_theme_map', &$map));
}
if (isset($mapids[$element['#map']])) {
drupal_set_message(t('Duplicate map detected! GMap does not support multiplexing maps onto one MapID! GMap MapID: %mapid', array('%mapid' => $element['#map'])), 'error');
// Return the div anyway. All but one map for a given id will be a graymap,
// because obj.map gets stomped when trying to multiplex maps!
return $o;
}
$mapids[$element['#map']] = TRUE;
// Inline settings extend.
$o_script .= 'Drupal.extend({ settings: '. drupal_to_js(array('gmap' => array($element['#map'] => $map))) ." });\n";
drupal_add_js($o_script, 'inline');
return $o;
}
There is a code in the middle, which is not mentioned above. When I removed this code, it didn't help.
#25
gmap-6.x-1.x-dev.tar.gz works for me!
#26
The patch from #11 worked for me as well.
#27
Not yet committed?
#28
@gagarine: No - we need to find a way to get in touch with the maintainer (bdragon). He hasn't been replying in the issue queue for the past few months.
#29
I have applied the patch #11, but the javascript error is still there. I can see the script tag and "CDATA" text in the source. It looks like it has not been updated, but the module is definitely patched and I cleared the cache using in Admin->Performance menu. Does anyone have a guess what might be a problem?
#30
Same problem as desecho ! Any guess ?