Move configuration form link and page to admin/config/development/jquery_update [#1449574]

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

Comment	File	Size	Author
#7	jquery_update-configuration_page-1449574-7.patch	2.94 KB	dqd
#7
#4	jquery_update-configuration_page-1449574-4.patch	2.94 KB	dqd
#4
#1	jquery_update-configuration_page-1449574-2.patch	2.85 KB	dqd
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

dqd

(formerly "diqidoq")

CreditAttribution: dqd commented 22 February 2012 at 01:04

File	Size
jquery_update-configuration_page-1449574-2.patch	2.85 KB

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...)

Comment #2

dqd

(formerly "diqidoq")

CreditAttribution: dqd commented 22 February 2012 at 01:04

Status:

Active

» Needs review

status ...

Comment #3

cweagans

He/Him

English

Boise, ID, USA

CreditAttribution: cweagans commented 22 February 2012 at 01:36

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)

Comment #4

dqd

(formerly "diqidoq")

CreditAttribution: dqd commented 22 February 2012 at 02:41

Status:

Needs review

» Needs work

File	Size
jquery_update-configuration_page-1449574-4.patch	2.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 ...

Comment #5

dqd

(formerly "diqidoq")

CreditAttribution: dqd commented 22 February 2012 at 02:20

Status:

Needs work

» Needs review

Comment #6

cweagans

He/Him

English

Boise, ID, USA

CreditAttribution: cweagans commented 22 February 2012 at 02:42

+++ 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'

Comment #7

dqd

(formerly "diqidoq")

CreditAttribution: dqd commented 22 February 2012 at 02:53

Status:

Needs work

» Needs review

File	Size
jquery_update-configuration_page-1449574-7.patch	2.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.

Comment #8

cweagans

He/Him

English

Boise, ID, USA

CreditAttribution: cweagans commented 22 February 2012 at 02:56

Status:

Needs review

» Reviewed & tested by the community

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

Comment #8.0

cweagans

He/Him

English

Boise, ID, USA

CreditAttribution: cweagans commented 22 February 2012 at 02:56

Issue summary:

View changes

add info

Comment #9

RobLoach

he/him

CreditAttribution: RobLoach commented 7 March 2012 at 15:28

Status:

Reviewed & tested by the community

» Fixed

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

Added a hook_help for the administration page. Nothing wrong with more user-facing documentation.
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.