Problem/Motivation

It is currently possible to override the jQuery version for admin pages, so that e.g. the default version for a site is jQuery 1.6, whereas on admin pages version 1.5 should be used. This solution is not consistent. The basic assumption seems to have been that admin pages use a different theme than other pages and so, this has been a solution to support modern fronted themes while still having a functional backend theme with an older version of jQuery.
Problems arise because of a conceptual mixup of admin pages and themes. Admin pages can be declared by any module. But that does not mean that those declared pages are necessarily rendered using the backend theme, as any module can also override the used theme.
An example of this: On the appearance settings page an administrator can select "Use the administration theme when editing or creating content". When this option is checked the backend theme is used for pages that are not considered admin pages and will not use the jQuery version usually associated to admin pages.

Proposed resolution

Drop the concept of jQuery version override for administrative pages and instead allow a general per-theme basis override of the used jQuery version. This should be done as an option on a themes settings page.

Remaining tasks

Review patch from #1969244-37: Specify jQuery version per theme.

User interface changes

On the jquery_update settings page: The proposed patch drops the simple version override selector and adds a table that shows per theme override settings. Part of the table is a link to a themes settings page where the jQuery version can be overridden.
On the settings page of every enabled theme: A new fieldset with a version selector including an option to use the default version that is set on the jquery_update settings page.
Example images:
https://drupal.org/files/issues/jquery_update-1969244-admin-settings.png
https://drupal.org/files/issues/jquery_update-1969244-theme-settings.png

#1524944: Allow different version for administrative pages
#1189496: Administrative and Views compatibility suggestion
#2141205: false positive on administration theme

Original report by emcniece

Some excellent work as been done in the thread over here, and having the ability to assign different jQuery versions for the administration and theme is extremely useful. There are a few cases in which the front-size theme and the administration overlap however.

Depending on how a site has its permissions set up, users may be able to edit their profile and account while still using the front-side theme... for example, /user/###/edit and /user/###/edit-profile. It looks like the system still counts these as "administration pages" and loads the administration-selected jQuery version. Unfortunately for me, this happens to be jQ 1.5 and I'm using the $.on() function in several places - conflict city.

There are several solutions to this - I could use $.live() instead of $.on(), or I could cut out the jQuery Update plugin altogether. There is probably an opportunity to consider a change in the way Drupal loads the jQuery for theme here as well, and I'm curious as to whether it warrants an analysis of how this module interacts and overrides based on path or template.

Bug report? Feature request? I'm not really sure... just a report for now.

Comments

berliner’s picture

Title: jQuery reverts on user account edit pages » Wrong jQuery version on some pages, depending on permissions and settings

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.

emcniece’s picture

It seems like we need a combination of the two systems - jQuery could be updated in the admin-side UNLESS the theme/template is rendering the page. This way, the jQuery Update module can still perform its admin-side updates without interfering with the front-facing theme.

I imagine that this could be coded in template.php, but perhaps there is also an opportunity to drop a checkbox in the jQuery Update admin page to allow theme jQuery override.

berliner’s picture

Status: Active » Needs review
FileSize
5.55 KB

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.

Paracetamol’s picture

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.

emcniece’s picture

@Paracetamol agreed - Fancybox is a major pain, since it requires jQuery 1.7 or higher... even when jQ1.7 breaks several admin interfaces (Views and Panels I think).

Hooking into the js override is easy enough... for programmers :). Magic does seem to be a nice addition, though some may consider an extra module like that overkill. The question now seems to be whether we're picky enough to have jQuery Update equipped with this feature at risk of bloat and redundancy.

Having @berliner's system of assigning a jQuery version per theme is nice if you have multiple active themes. But I guess even the admin theme is a theme on its own, right? So you always have at least 2 active themes... yeah, this makes sense to break jQuery assignment out. This way, you don't have to worry about exactly what page is considered an "admin page"... instead, it only matters what theme is currently active.

Makes a lot of sense to me!

berliner’s picture

Issue summary: View changes

Updated issue summary

berliner’s picture

Issue summary: View changes
berliner’s picture

@emcniece: Could you review this patch?

emcniece’s picture

- l(t('Change'), 'admin/appearance/settings/' . $theme_name, array('fragment' => 'edit-jquery-update-version', 'query' => drupal_get_destination())), might wrap at 80 characters (if you're picky)

- in jquery_update_form_system_theme_settings_alter(), should $theme_name be obtained by $form_state['build_info']['args'][0] instead of preg_replace('[^theme_|_settings$]', '', $form['var']['#value'])? Not sure which method is more reliable.

Excellent work, switching versions between themes is a breeze! This also performs properly on exposed admin forms, as expected.

berliner’s picture

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.

berliner’s picture

Issue summary: View changes
berliner’s picture

Issue summary: View changes
berliner’s picture

Uploaded some screenshots.

berliner’s picture

Issue summary: View changes

Added related issue and fixed typo.

emcniece’s picture

[delete]

emcniece’s picture

Would love to see this get some attention, I think this should be committed.

trevorkjorlien’s picture

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!

emcniece’s picture

Awesome!!

berliner’s picture

Status: Needs review » Reviewed & tested by the community
pbuyle’s picture

Status: Reviewed & tested by the community » Needs work

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.

pbuyle’s picture

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

pbuyle’s picture

Status: Needs work » Needs review
berliner’s picture

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,
pbuyle’s picture

Status: Needs review » Reviewed & tested by the community

Patch in #22 works great for me.

emcniece’s picture

Confirming #22

gmclelland’s picture

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"?

berliner’s picture

Title: Wrong jQuery version on some pages, depending on permissions and settings » 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.

ezheidtmann’s picture

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

tparc’s picture

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

berliner’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
6.64 KB

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?

ckng’s picture

Status: Needs review » Reviewed & tested by the community

#29 works great.

markcarver’s picture

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;
    }
    
berliner’s picture

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

berliner’s picture

Status: Needs work » Needs review
FileSize
6.9 KB
5.29 KB
  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
berliner’s picture

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

markcarver’s picture

Assigned: Unassigned » markcarver

I will review this later today.

markcarver’s picture

Assigned: markcarver » Unassigned
FileSize
10.62 KB
10.48 KB

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.

berliner’s picture

FileSize
10.16 KB
322 bytes

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.

markcarver’s picture

Status: Needs review » Reviewed & tested by the community

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.

ckng’s picture

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

berliner’s picture

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?

berliner’s picture

Issue summary: View changes
hanoii’s picture

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.

JoshRickert’s picture

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.

Paracetamol’s picture

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!

markcarver’s picture

Version: 7.x-2.x-dev » 7.x-3.x-dev
Category: Bug report » Feature request
Status: Reviewed & tested by the community » Fixed

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.

  • markcarver committed ad206aa on 7.x-3.x
    Issue #1969244 by berliner, markcarver, mongolito404: Specify jQuery...
berliner’s picture

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

pbuyle’s picture

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?

markcarver’s picture

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.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.