Posted by kkaefer on August 1, 2006 at 8:22pm
| Project: | Drupal core |
| Version: | x.y.z |
| Component: | base system |
| Category: | task |
| Priority: | normal |
| Assigned: | kkaefer |
| Status: | closed (fixed) |
Issue Summary
As more JavaScript enabled modules are available, it's quite a problem to not get collisions between their configuration variables. This patch provides a centralized place where modules can put their settings that are required for the current page by putting them in the global (JavaScript) variable Drupal.
This patch also cleans up the act of adding a JavaScript file to the page by exposing the HTML code in a theme function for easy manipulation.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| js_settings.patch | 4.49 KB | Ignored: Check issue status. | None | None |
Comments
#1
Tweaked it a bit to use Drupal.settings instead of Drupal. Removed the theme function as script tags don't need to be themed.
#2
(Changed title to better reflect what this patch does)
#3
big +1 to the concept from me.
this will make moving the chatroom module to jQuery and HEAD easier and cleaner.
i'll review the code this weekend.
#4
another +1 to the concept from me.
this will also make the 4.7 update for AJAX Spellcheck and the Javascript Tools JS insertion easier and cleaner.
hope to find time to review the patch later.
#5
Rerolled against HEAD (due to http://drupal.org/node/73961).
#6
Changed the patch in huge parts. It now uses the same system as drupal_add_css() (different 'layers').
Here is the (very comprehensive) documentation for the new
drupal_add_js<code>: (Note that the function itself is not that complex)
<code>The behavior of this function depends on the parameters it is called with. Generally, it handles
the addition of JavaScript to the page, either as reference to an existing file or as inline code.
The following actions can be performed using this function:
- Add a file ('core', 'module' and 'theme'):
Adds a reference to a JavaScript file to the page header. JavaScript files are placed in a certain order
from 'core' first, to 'module' and finally 'theme' so that files that are later added, can override
previously added files with ease.
- Add inline JavaScript code ('page'):
Executes a piece of JavaScript code on the current page by placing the code directly in the head of
the page. This can, for example, be useful to tell the user that a new message arrived using a pop up,
alert box or similar method.
- Call a JavaScript function ('call'):
Generates a Javascript call, while importing the arguments as is. PHP arrays are turned into JS objects
to preserve keys. This means the array keys must conform to JS's member naming rules.
- Add settings ('setting'):
Adds a setting to Drupal's global storage of JavaScript settings. Per-page settings are required by
some modules to function properly. The settings will be accessible at Drupal.settings[$flag].
@param $data
(optional) If given, the value depends on the $type parameter:
- 'core', 'module' or 'theme': The path to the JavaScript file relative to the base_path().
- 'page': The JavaScript code that should be placed directly in the head of the page.
- 'setting': An array with configuration options as associative array.
- 'call': The name of the function that should be called on page load.
@param $type
(optional) The type of JavaScript that should be added to the page. Allowed values are 'core',
'module', 'theme', 'page', 'setting' and 'call'.
@param $flag
(optional) The value of this parameter depends on the $type parameter:
- 'core', 'module', 'theme' and any undefined value: Sets whether the JavaScript file should be
cached (TRUE, default value) or loaded anew on every page (FALSE).
- 'setting': The name of the module that adds settings to the page.
- 'page' and 'call': The value should be omitted.
@param ...
(optional) If the parameter $type is set to 'call', every further parameter after $type is passed
as an argument to the function called in the page header.
@return
The JavaScript array that has been built so far.
#7
Fixed a misnamed variable. In one place, I had 'arguments' and in the other, I had 'parameters'.
I forgot to mention that I replaced
drupal_call_js()withdrupal_add_js('function', 'call' [, parameters]).#8
Argh, patch didn't get attached. Here it is.
#9
I haven't actually tried this patch yet, but having dynamic adding of css and javascript files behave in the same manner is definitely going to make it easier for developers.
#10
Normally we wrap PHPDoc comments around column 80. This is not a strict requirement, but your lines are kinda long. They hardly fit on my 12" screen.
I'm not a JavaScript expert so I can't comment on the functionality itself, or how convenient the various optional paramaters combinations are.
#11
Made the PHPDoc comment wrap at column 80.
#12
I *really* like this patch. Don't have time to thoroughly test it, but code looks great, awesome job!
#13
Whats all this setting and call stuff?
#14
The call function has been there before, as
drupal_call_js(). It is required by some contributed modules. The settings function is pretty important for future JavaScript modules. See for example http://drupal.kkaefer.com/ for an example of how settings can be used.#15
Rerolled against HEAD. Changed
array_mergetoarray_merge_recursiveto prevent overwriting settings.#16
Patch still applies.
#17
code looks good, and this patch worked as advertised for adding files, adding config vars, adding js and calling functions with and without args.
good work tim!
don't have enough karma around here to set it to RTBC, but it looks good to go.
this patch still leaves blocks unable to add js for all core themes, but that's a different issue.
#18
First off, I really like this patch, makes things consistent with drupal_add_css().
Next, timcn, you forgot your settings.php in there ;-)
Next up, in this case:
case 'setting':$output .= '<script type="text/javascript">if (typeof Drupal == "undefined") Drupal = {}; Drupal.settings = '. drupal_to_js($data) .";</script>\n";
break;
What if mutlipe JS files define settings? That seems to overwrite each other.
Also, what about Google Analytics? Why? Well, that places code that goes in the *footer*, just before , same with other modules that place JS in the $closure. How would that work?
And what if a module wants to output Javascript in the page, like TinyMCE? Should it use drupal_add_js()?
Inline seems like it places JS within the page, not within the head. Maybe make the distinction between JS files in head and actual JS code in head, verse JS code in the body?
Otherwise, looks great!
#19
I actually can't find any settings.php rules in the patch.
In
drupal_add_js(), no HTML code is generated. The values are just merged into the global (static actually) settings array. The code for settings is generated in drupal_get_js() when all modules have merged their settings into the array. That means, all settings are there. I am successfully running this patch with quite some JavaScript code (from different modules) for my SoC project.You're right. There is definitely a need to add JavaScript code to the $closure. We should figure out, in what places it makes sense to position JavaScript code.
#20
Timnc, yeah sorry about the settings.php comment, no idea where that came from. Clearly not in there :-)
As for including multiple JS files, what I was talking about was:
case 'setting':$output .= '<script type="text/javascript">if (typeof Drupal == "undefined") Drupal = {}; Drupal.settings = '. drupal_to_js($data) .";</script>\n";
I see that it'll apend multiple script tags no problem. However, this part:
Drupal.settings = '. drupal_to_js($data), each subsequent script will overwrite that, no?#21
Updated the patch. Note that settings was not broken before. The arrays were merged together so that only one
Drupal.settingswas required.Changes
'header', but there is also a scope called'footer'available. Themes can define and use arbitrary scopes.$type = 'call'as it is probably easier to writedrupal_add_js('alert("message")', 'inline');than using the parameter construction.pagetoinlineas it is easier to understand.settingtype:Drupal.settingsnamespace.array_merge_recursiveis now only called once. Before, it was called whenever you added a setting.deferthe execution of JavaScript code. Take a look at the MSDN page if you don’t know what that means.$flagsis not overloaded anymore.#22
I forgot to mention that I will work on JavaScript localization once this patch is in core.
#23
Damn, forgot a
print_r()in the code.#24
Timcn, excellent job! You addressed all of my concerns with the patch.
Looking through it now, looks very solid. This will go a long way to unifying a consisten approach to adding JS to a Drupal page, much like drupal_add_css() did this for CSS.
I'd say it's RTBC, but maybe someone else wants to have a review first? Thumbs up here.
#25
i'll review and test this tonight. (GMT +10 Sydney)
#26
Rerolled the patch.
#27
tested again, and it still works as advertised.
i think the docs need an update here though:
* - 'inline': The JS code that should be placed directly in the page head.this needs to take into account the $scope param, and the fact that the code might not be placed in the page head.
nice work :-)
#28
You're right. I changed the documentation for 'inline'. Thanks for testing.
#29
Fixed a wrong indentation in drupal.js and took a file out that has nothing to do here ;)
#30
Rerolled the patch to remove fuzziness. Setting this RTBC.
#31
Agreed, we need this to go in before JQuery hits... this will really help out.
#32
Committed to CVS HEAD. Marking this 'code needs work'. When this is properly documented, switch to 'fixed'. Thanks timcn! :)
#33
looks like dries forgot to change the status...
#34
As per Steven's request, here is a documentation patch for
Drupal.extend().#35
As per Steven's request, here is a documentation patch for
Drupal.extend().#36
A bit wordy "Arbitrary objects containing for example functions or variables can be used. " ... how about "Arbitrary objects, such as functions or variables, can be used."
#37
Well, actually objects can contain functions and variables. While functions are also objects, variables are not.
#38
Updated the module upgrade page.
Updated the documentation patch with m3avrck's suggestion.
#39
Looks good now.
#40
Steven will have to commit this -- I don't know what his requests were.
#41
This looks straightforward enough. What/where were Steven's requests?
#42
I was informed via IRC that Steven simply wanted documentation.
Committed to HEAD.
#43
Just discovered that a variable has been misnamed.
#44
Committed to CVS HEAD.
#45