Specify jQuery version per theme

I'm generally in favor of the feature developed in #1524944: Allow different version for administrative pages, but I think it's wrong to depend on the admin paths. Just like emcniece I have run into difficulties.

On the general theme settings page (/admin/appearance) is the checkbox "Use the administration theme when editing or creating content". When you have two different themes for front- and backend and you uncheck this option, then this effectively gives you the wrong jQuery version. So I would vote in favor of comment #1524944-108: Allow different version for administrative pages who proposes to make the jQuery Version theme dependent. That would also allow for more than two themes running on different version of jQuery, just think of the domain module and the possibility to have several different section on the site that run different themes.

I can't really evaluate the need for the last possibilities I have mentioned. But I would strongly favor for a more coherent approach. The current solution seems counter intuitive even assuming there is one theme for the frontend and one for the backend, each one requiring a different version. Then this can break functionality on the node edit page in one of the two themes if the before mentioned option is set.

Why not depend directly on the used theme? After all, jQuery is a theme related thing. I could not care less if I'm on an admin page or not, as long as the theme functions correctly.

Actually I was more thinking about something along the lines:

• Have one default setting for the jQuery version to use
• Allow to override the jQuery version on a per-theme basis on each enabled themes settings page
• Provide an overview of the overwrites on the jquery_update settings page

See the attached patch for my idea on this case. Using this approach the admin version can be completely removed, as this applies only when you have selected a different theme for admin pages, and thus the jQuery version to use on this theme can be selected in the settings for this theme.

If this would to be included it would be easy too, to write an update routine to keep the current settings. But I didn't include that in the patch yet. Firstly, it will be interesting to see what others are thinking about this.

Just a quick chime-in – it is possible to override the jQuery version via theme – I don't know, if it works as a normal theme.info file override, but there are certainly ways to do this via simple template routines or modules like Magic.
There is a lengthy wiki page on this topic (mainly for Drupal 6, if I recall) as well.

What I find much more problematic is that certain modules (Fancybox comes to my mind) __depend__ on activating the jquery update module, which itself is so elaborate, that it kills all other practical override approaches.

Updated issue summary

@emcniece: Could you review this patch?

Thanks @emcniece for the review.
I have updated the patch to be compatible with the latest dev version.

Please, if anybody could review this a little more and change the status to "Reviewed & tested by the community"? Also it would be good to hear something from the maintainers to know if they are generally in favor of this approach.

Uploaded some screenshots.

Added related issue and fixed typo.

Applied patch to the latest dev version of jQuery Update (7.x-2.3+15-dev) with no errors. Changing jQuery versions was straight-forward and worked great!

jquery_update_get_theme_settings() does not retrieve settings set in the theme .info file. Only the jquery_update_version key of the theme settings retrieved by jquery_update_get_theme_settings() are used. theme_get_setting() could be used to retrieve this settings, therefor the jquery_update_get_theme_settings() function is redundant and useless.

The attached patch remove jquery_update_get_theme_settings() and use theme_get_setting() instead.

Good catch.
Man, you're fast! I was just about to upload a corrected patch.

Just a small thing:

'#default_value' => empty($theme_version) ? '' : $theme_version,

can be simplified to:

'#default_value' => $theme_version,

Patch in #22 works great for me.

This may be need to be updated if #1548028: Make the default jQuery version (1.4.4) for D7 an option gets committed.

I almost missed this issue. Should this issue be renamed to "specify Jquery version per theme"?

Good idea for the title!
I will have a look at #1548028: Make the default jQuery version (1.4.4) for D7 an option to see how that affects the patch. It's definitely something I wondered about before, so glad to see that this is advancing too.

Confirming #22 works great for me. Thanks y'all!

Are there any plans to have this committed to the latest build?

Updated the patch to be compatible with the latest dev. Also included default setting from #1548028: Make the default jQuery version (1.4.4) for D7 an option.

Any feedback from the maintainers?

#29 works great.

re #29:

Any feedback from the maintainers?

First, let me say that I think this is definitely a great approach! Here is, however, some feedback on some coding standards, "Best Practices™" and general architecture decisions:

1. +++ b/jquery_update.module
   @@ -75,29 +75,30 @@ function jquery_update_library() {
   +  global $theme;
   ...
   +    $theme_version = theme_get_setting('jquery_update_version', $theme);
   

   There's no need for global $theme; here. theme_get_setting() defaults to the current theme if none is provided.

   Furthermore, I would prefer if we followed the same variable naming conventions here: jquery_update_jquery_version.

2. +++ b/jquery_update.module
   @@ -75,29 +75,30 @@ function jquery_update_library() {
      $version = variable_get('jquery_update_jquery_version', '1.10');
   ...
   -  if ($module === 'system') {
   +  if ($module === 'system' && $version) {
   

   Why is this change needed? It defaults to 1.10, so there is always a value.

3. +++ b/jquery_update.module
   @@ -75,29 +75,30 @@ function jquery_update_library() {
   -    $admin_version = variable_get('jquery_update_jquery_admin_version', '');
   ...
   -    if (!empty($admin_version) && path_is_admin(current_path())) {
   -      if (version_compare($version, $admin_version, '!=')) {
   -        $version = $admin_version;
   +    $theme_version = theme_get_setting('jquery_update_version', $theme);
   +    if ($theme_version) {
   +      if (version_compare($version, $theme_version, '!=')) {
   +        $version = $theme_version;
   

   Most themes are what usually control jQuery in the end. The introduction of #1524944: Allow different version for administrative pages was the predecessor to this issue and has definitely helped eliminate some issues where certain contrib module (like Views) broke rather severely with higher jQuery versions.

   While this feature is relatively new, it is in a stable/production ready release. Thus, since this patch effectively is removing this feature, it now needs an update hook. It should migrate the old admin variable and also remove/uninstall it.

   For the update, I would suggest that if the admin_theme === 'seven' and there is a value for jquery_update_jquery_admin_version, then that value should be used for the seven theme's "override". If it isn't set, we should probably default to 1.5.

4. +++ b/jquery_update.module
   @@ -75,29 +75,30 @@ function jquery_update_library() {
   +    if ($theme_version) {
   +      if (version_compare($version, $theme_version, '!=')) {
   +        $version = $theme_version;
          }
        }
   

   We should reduce this to a single if statement using the && operator.

5. +++ b/jquery_update.module
   @@ -170,36 +171,60 @@ function jquery_update_settings_form() {
   +  $header = array(t('Theme'), t('Status'), t('jQuery version'), t(''));
   

   Don't enclose an empty string with t().

6. +++ b/jquery_update.module
   @@ -170,36 +171,60 @@ function jquery_update_settings_form() {
   +  ¶
   

   Whitespace

7. +++ b/jquery_update.module
   @@ -170,36 +171,60 @@ function jquery_update_settings_form() {
   +  $form['version_options']['themes'] = array(
   

   If one is altering this form an encounter "version_options", what is it for? This should really just be $form['jquery_update']['themes']. edit: Since I was just reviewing the patch in browser, I lost the context. This is in jquery_update_settings_form(), so my suggestion would be instead to just simplify to versions instead.

8. +++ b/jquery_update.module
   @@ -170,36 +171,60 @@ function jquery_update_settings_form() {
   +    '#title' => t('Theme specific versions'),
   

   Just rename to read "jQuery version". edit: Ignore this. I again, lost context here. This is appropriate for the jQuery Update settings form.

9. +++ b/jquery_update.module
   @@ -170,36 +171,60 @@ function jquery_update_settings_form() {
   +  $form['version_options']['themes']['overrides'] = array(
   +    '#type' => 'markup',
   +    '#markup' => theme('table', array('header' => $header, 'rows' => $rows)),
      );
   

   Don't use #markup and force the rendering of the table now using theme(). This doesn't allow the table in the form to be altered by anything else. Instead you should render the table like:

   $form['jquery_update']['themes']['overrides'] = array(
     '#theme' => 'table',
     '#header' => $header,
     '#rows' => $rows,
   );
   

10. +++ b/jquery_update.module
    @@ -240,6 +265,55 @@ function jquery_update_settings_form() {
    +  $theme_name = $form_state['build_info']['args'][0];
    ...
    +  $theme_version = theme_get_setting('jquery_update_version', $theme_name);
    

    Rename $theme_name to $theme_key. This is the naming convention used through-out most of core and contrib.

11. +++ b/jquery_update.module
    @@ -240,6 +265,55 @@ function jquery_update_settings_form() {
    +  $themes = list_themes();
    ...
    +      '%theme_name' => $themes[$theme_name]->info['name'],
    

    There is no need for list_themes() here. Just use the $theme_info global.

12. +++ b/jquery_update.module
    @@ -240,6 +265,55 @@ function jquery_update_settings_form() {
    +  $form_key = $theme_name . '_jquery_update';
    +  $form['options_settings'][$form_key] = array(
    

    This structure seems overly complex. options_settings is not used by core and again, runs the risk of not being sure what it is (due to a vague naming convention) during a future form alter.

    I would instead just use $form['jquery_update'] as the top level fieldset hierarchy.

13. +++ b/jquery_update.module
    @@ -240,6 +265,55 @@ function jquery_update_settings_form() {
    +  $form['options_settings'][$form_key]['jquery_update_version'] = array(
    

    Same goes for here, I would mimic the theme setting/variable name: jquery_update_jquery_version.

14. +++ b/jquery_update.module
    @@ -240,6 +265,55 @@ function jquery_update_settings_form() {
    +    '#title' => t('jQuery version for theme %theme_name', array(
    +      '%theme_name' => $themes[$theme_name]->info['name'],
    +    )),
    

    I would limit this to simply say: "Theme specific jQuery version"

15. +++ b/jquery_update.module
    @@ -240,6 +265,55 @@ function jquery_update_settings_form() {
    +function jquery_update_get_versions() {
    +  return array(
    +    '1.5' => '1.5',
    +    '1.7' => '1.7',
    +    '1.8' => '1.8',
    +    '1.9' => '1.9',
    +    '1.10' => '1.10',
    +  );
    +}
    

    It would be far better to use drupal_map_assoc() here. Also, we should make this static so it's not initializing an array each time it's called (very minor, but every little bit helps when it comes to performance).

    function jquery_update_get_versions() {
      static $versions = drupal_map_assoc(array('1.5', '1.7', '1.8', '1.9', '1.10'));
      return $versions;
    }
    

Great feedback! Thanks for the review. I'll see to update the patch with the proposed changes in the next few days.

1. fixed
2. fixed
3. Added update hook
4. fixed
5. fixed
6. fixed
7. The form element is already named version_options in the current code, so I would refrain from changing it to reduce noise in the patch
8. Nothing to do
9. fixed
10. fixed
11. The global variable is not an option are, because it holds the theme used to edit the theme settings page. But I removed the use entirely.
12. $form['jquery_update']['jquery_update_jquery_version'] = array(
    Aggreed. I also removed the intermediary form key as it's not necessary as far as I can tell.
13. fixed
14. fixed
15. Agreed and fixed

Sorry, I misnamed the patch file. Should be 33 at the end.

I will review this later today.

I started to reply, but found it difficult to really explain what I was trying to get across. So here is a patch with an interdiff. Doing this also allowed me to really look over this in great detail and verify that this actually works as expected.

I'd like you to look it over and see if the changes I have made make any sense. Please feel free to submit a new patch in response if necessary.

Your changes seem reasonable for the most part. You're certainly trying to be more verbose and to clean up semantics in the code besides adding the basic feature that this issue is about. I tried to follow what I thought was the usual process for patches: Try to limit changes to the necessary in order to keep the noise low.

+function _jquery_update_set_theme_version($theme_key, $version) {

I'm personally not in favor of starting functions with an underscore to mark them private, but I guess this is more of a taste thing and seems to be quite common in contrib.

Apart from that I actually like your changes on the first glance.
I just did a minor correction regarding the fragment part of the configure link.

Btw.: Are you a maintainer of this module? I can't see you in the list.

You're certainly trying to be more verbose and to clean up semantics in the code besides adding the basic feature that this issue is about.

Yes. This is partially because we're not just adding a feature, we are changing how jquery_update fundamentally works (and removing the previous "admin version" feature btw). We should be more verbose and indeed clean up some things that no longer make sense with the introduction of this issue/patch.

+function _jquery_update_set_theme_version($theme_key, $version) {
I'm personally not in favor of starting functions with an underscore to mark them private, but I guess this is more of a taste thing and seems to be quite common in contrib.

Yes, it's definitely a Drupalism/convention (see https://www.drupal.org/node/70335). The main reason I decided this was two fold: 1) It's in a .install and is only used here 2) Furthering that reason, I did not want to confuse anyone because it isn't used by the main module (ie: when a theme sets it's version on the theme settings page). It's merely meant for a one time use: install or update.

Btw.: Are you a maintainer of this module? I can't see you in the list.

Yes. I am a new co-maintainer. I haven't actually committed any code myself yet, which is why you don't see me on the list. However you can see it here: https://www.drupal.org/node/139405/commits

Based on your feedback, I'm going to mark this as RTBC. But I'd like to wait a bit to get others feedback before we proceed forward.

One possible issue I noticed is settings[jquery_update_jquery_version] in theme.info is not being picked up.

It is not? I just tested this with a new clean install and it is working for me. But since you can override it on the settings page of a theme, it will be stored in the database once the settings form is submitted the first time. Any following changes to settings[jquery_update_jquery_version] in theme.info won't have any effect.

How is that supposed to work? I would guess that this is the same behavior for all theme settings?

It's amazing when you think of something, you search for it, you reach an issue in drupal.org and is as good as this one is. Working perfectly for me and a big +1 for this issue.

Agreed. I'm running it too and it's working great. Solved an "undefined function" error where the (themed) user/###/edit page was receiving the "admin" jQuery version and causing errors with bootstrap.

Just confirming, that this works – one welcome feature (in expectation of Drupal 8) would be to disable jQuery altogether on a certain theme – tell me, if this goes into another feature request.

Thanks for the good work!

Ok good, we've gotten enough reviews for my taste.

Since this is technically a feature request and such a major shift in how this module operates from previous versions, I really think this should be the starting point of a new 7.x-3.x branch.

I've gone ahead and created this new branch and committed this patch there. I will release a new version of 7.x-2.5 to push out the commits that are sitting on the 7.x-2.x branch, but any further development should happen on the 7.x-3.x branch.

Nice. I'm happy to see that this is in now.

This is a major feature, so I understand and welcome the switch to 3.x. Can we expect a 3.0 release soon after the 2.5 release, so we can have a stable release with this feature?

There are still a few other issues that I would like to get in first (I've already marked them as 7.x-3.x) before we make a release.