After chatting with RobLoach and cweagans on IRC, we came to the agreement, that the configuration form link should rather point to admin/config/development/jquery_update instead of to the development/performance forms, like it is atm. This needs another hook_form(), since jQuery update alters the form of development performance in the moment.

Ok, here we are. Patch follows in the next hours

Comments

diqidoq’s picture

And here it is ...

(since this is an own configuration page now, it needed another hook_menu and permission implementation, and the form isn't altering another form no more, so this part needed small changes like function rename and return system_settings_form($form) etc...)

diqidoq’s picture

Status:Active» Needs review

status ...

cweagans’s picture

Status:Needs review» Needs work
+++ b/jquery_update.moduleundefined
@@ -81,10 +81,36 @@ function jquery_update_library_alter(&$javascript, $module) {
/**
+ * Implements hook_permission().
+ */
+function jquery_update_permission() {
+  return array(
+    'access jQuery update' =>  array(
+      'title' => t('administer jQuery update configuration'),
+    ),
+  );
+}

This should probably be 'administer jquery update configuration' (all lowercase for consistency). The use of the word 'administer' is also consistent with our other admin pages.

+++ b/jquery_update.moduleundefined
@@ -81,10 +81,36 @@ function jquery_update_library_alter(&$javascript, $module) {
+/**
+ * Implements hook_menu().
+ */
+function jquery_update_menu() {
+  $items['admin/config/development/jquery_update'] = array(
+    'title' => 'jQuery update',
+    'description' => 'Upgrades the version of jQuery in Drupal core to a newer version of jQuery.',
+    'page callback' => 'drupal_get_form',
+    'page arguments' => array('jquery_update_settings'),
+    'access arguments' => array('access jQuery update'),
+  );
+
+  return $items;

Don't forget to change the permission name here too. The description could probably use a bit of work: the admin page itself doesn't upgrade jQuery (the module does that). The admin page allows users to configure settings related to the jQuery upgrade. Not sure how to phrase that, though.

+++ b/jquery_update.moduleundefined
@@ -81,10 +81,36 @@ function jquery_update_library_alter(&$javascript, $module) {
-  $form['bandwidth_optimization']['jquery_update_jquery_version'] = array(
+function jquery_update_settings() {
+  $form['jquery_update_jquery_version'] = array(
     '#type' => 'select',
     '#title' => t('jQuery Version'),
     '#options' => array(
@@ -94,7 +120,7 @@ function jquery_update_form_system_performance_settings_alter(&$form, &$form_sta

@@ -94,7 +120,7 @@ function jquery_update_form_system_performance_settings_alter(&$form, &$form_sta
     '#default_value' => variable_get('jquery_update_jquery_version', '1.5'),
     '#description' => t('Select which jQuery version branch to use.'),
   );
-  $form['bandwidth_optimization']['jquery_update_compression_type'] = array(
+  $form['jquery_update_compression_type'] = array(
     '#type' => 'radios',
     '#title' => t('jQuery compression level'),
     '#options' => array(
@@ -103,7 +129,7 @@ function jquery_update_form_system_performance_settings_alter(&$form, &$form_sta

@@ -103,7 +129,7 @@ function jquery_update_form_system_performance_settings_alter(&$form, &$form_sta
     ),
     '#default_value' => variable_get('jquery_update_compression_type', 'min'),
   );
-  $form['bandwidth_optimization']['jquery_update_jquery_cdn'] = array(
+  $form['jquery_update_jquery_cdn'] = array(
     '#type' => 'select',
     '#title' => t('jQuery and jQuery UI CDN'),
     '#options' => array(
@@ -115,6 +141,8 @@ function jquery_update_form_system_performance_settings_alter(&$form, &$form_sta

@@ -115,6 +141,8 @@ function jquery_update_form_system_performance_settings_alter(&$form, &$form_sta
     '#default_value' => variable_get('jquery_update_jquery_cdn', 'none'),
     '#description' => t('Use jQuery and jQuery UI from a CDN. If the CDN is not available the local version of jQuery and jQuery UI will be used.'),
   );
+
+  return system_settings_form($form);
}

/**

This should probably be jquery_update_settings_form() (so that we know it's a form builder function)

diqidoq’s picture

Status:Needs review» Needs work
StatusFileSize
new2.94 KB

cweagans: that the access argument value needs to be identical to the permission array name is clear, for sure. But your example text in the first comment ( "This should probably be 'administer jquery update configuration' (all lowercase for consistency)") is confusing, since you use the title as an example, not the array name.

Also I wouldn't agree with changing a readable title to another name convention of jQuery than jQuery. So I think, you rather asked me to change the permission array name, which refers to the identical access argument value of course ;)

Yeah, the description doesn't exist before, and I took the first sentence from the module project page on d.o. - But I think your phrase suggestion is quite good already and I just added path and compression. ;)

I don't have a problem with jquery_update_settings_form().

Thanks for the fast review! cweagans++

So, ok, here it is so far ...

diqidoq’s picture

Status:Needs work» Needs review
cweagans’s picture

+++ b/jquery_update.moduleundefined
@@ -81,10 +81,36 @@ function jquery_update_library_alter(&$javascript, $module) {
+function jquery_update_permission() {
+  return array(
+    'access jquery update' =>  array(
+      'title' => t('administer jQuery update configuration'),
+    ),
+  );

Instead of 'access jquery update', use 'administer jquery update configuration'.

+++ b/jquery_update.moduleundefined
@@ -81,10 +81,36 @@ function jquery_update_library_alter(&$javascript, $module) {
+function jquery_update_menu() {
+  $items['admin/config/development/jquery_update'] = array(
+    'title' => 'jQuery update',
+    'description' => 'jQuery update allows users to configure settings related to the jQuery upgrade, the library path and compression.',
+    'page callback' => 'drupal_get_form',
+    'page arguments' => array('jquery_update_settings_form'),
+    'access arguments' => array('access jquery update'),
+  );
+
+  return $items;

To be in line with the previous change, replace 'access jquery update' with 'administer jquery update configuration'

diqidoq’s picture

Status:Needs work» Needs review
StatusFileSize
new2.94 KB

... aaah, now I got it! *lol*
you must know it's 3:44 in the morning here .... heh

also changed description to: "Configure settings related to the jQuery upgrade, the library path and compression." like spoken in IRC.

cweagans’s picture

Status:Needs review» Reviewed & tested by the community

Code looks good. Everything works as expected. Good job.

cweagans’s picture

Issue summary:View changes

add info

RobLoach’s picture

Status:Reviewed & tested by the community» Fixed

Thanks a lot for the help, guys!!! Looks great. Committed with two small changes:

  1. Added a hook_help for the administration page. Nothing wrong with more user-facing documentation.
  2. Removed hook_permission() and just used "administer site configuration" instead. It was using that permission before, if we want a specific permission for jQuery configuration, we could easily open up a new issue. Just don't like removing things for users who don't expect it.

Thanks so much!!! This is awesome.

Status:Fixed» Closed (fixed)

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

Anonymous’s picture

Issue summary:View changes

Updated issue summary.