Refactor system.js file and use Drupal form states and form ajax

Splitting up system.js sounds more appropriate to me too, since the functions are for specific system pages, rather than for the whole module.

Here's the list of the js functions and which PHP function build the relevant page:

Drupal.behaviors.cleanURLsSettingsCheck
Drupal.cleanURLsInstallCheck
  system_clean_url_settings()

Drupal.behaviors.copyFieldValue
  install.php

Drupal.behaviors.dateTime
Drupal.behaviors.userTimeZones
  system_regional_settings()

Drupal.behaviors.poweredByPreview
  system_block_configure

Drupal.behaviors.copyFieldValue may still be counted as a utility function. Even though it's currently only used by install.php, it can be used elsewhere. I didn't move that function in the following patch, but all the other ones have been moved to their own js file.

It would also mean that crazy bugs like this one - #479604: Drupal.behaviors.cleanURLsSettingsCheck() in system.js redirects to clean urls settings page incorrectly. - would no longer be possible.

Wrong. That bug is caused by a JavaScript bug and nothing in Drupal prevents a JavaScript from loading. Even if system.js gets splitted, a buggy JavaScript can still be loaded and break other functionality.

I'm not sure the proposal of this issue makes sense. As of now, system.js is only loaded conditionally where explicitly needed - and that's rare already.

However, it perhaps would make sense to introduce a new misc/install.js file to bundle all the special logic for install.php.

Tagging for feature freeze.

The tagging never happened. Is this still a go for D7?

Since this would just move code around, and even could count as performance improvement (since that JS for install.php would no longer be loaded/aggregated), I think so, yes.

I'm still not sure whether it should be misc/install.js or modules/system/system.install.js. I think the latter would be cleaner.

taking this one.

Here is a new patch, at the end instead of splitting system.js to multiple files I refactored that old/ugly js code in it - most of the stuff we can do now with states and form ajax. On the end we have only one behavior in new system.js which is used for now only by installer.

I need a review of changes in system_date_time_lookup() - had some problem with replace state - it is injecting one additional empty <div></div> on the beginning for some reason at that is messing layout a little bit, so I used ajax_command_replace() but maybe there is a smarter way for this.

The rest seems ok, tested in FF, no need to test in other browsers (IE) because there are no changes that are browser specific.

I also runned jshint on new system.js and applied recommendation from it, this change (and whole refactoring of system.js) is related to #1684872, so i guess this two patches should be merged into one.

Also we should mybe backport this to D7, because Drupal.behaviors.pageCache does not work in D7 also.

nod_ explain it to me why we should use function someFunction() instead of var someFunction = function(), so here is new patch with that change. But we still don't know why install is failing with testbot.

+++ b/core/includes/install.core.incundefined
@@ -1532,7 +1532,6 @@ function install_configure_form($form, &$form_state, &$install_state) {
   drupal_add_js(array('copyFieldValue' => array('edit-site-mail' => array('edit-account-mail'))), 'setting');
   // Add JS to show / hide the 'Email administrator about site updates' elements
-  drupal_add_js('jQuery(function () { Drupal.hideEmailAdministratorCheckbox() });', 'inline');

Is there still a point to this "Add JS to show / hide ..." comment line? Attached patch removes that.

Also: no clue why it fails on MySQL during install, seems unrelated to these changes. So, gogo testbot!

Yeah you are right we need to remove that comment also.

I really suck with testbot, and have no clue how to debug this.

Can anyone confirm this isn't PIRF tweaking out?

Might be related to the change of checkbox name, any way to keep the old values of the checkboxes?

No I don't think so, because we changed checkboxes to 2 checkbox - which we need because we want to use states on that two checkbox. I guess install test for bot is hardcoded somewhere else and not in drupal code - somebody need to change that install test script which will be bad because all other issues test will fail until this patch is applied to drupal.

I don't know what is a resolution process for this kind of patches - any idea?

What I meant was it should be possible to do, have a look into form_process_checkboxes() in form.inc an try to manually do that.

As i've learned the hard way, it's unlikely to be the testbot that fails :)

Okey here is slightly modified patch from 13 - same stuff basically but bot will maybe happy with this, lets wait and see...

Awesome :)

a few things and it'll be good to go:

The updateTarget function needs a parameter e so that you can refer to e.target instead of using this line 19.
There is a semicolon that needs to be removed line 23 as well.
You can remove context in the jQuery selector since we're looking for an ID.

That is a very very nice clean up :)

Fixed JSHint objections and removed context as per #21

OK here is new patch based on hints from 21. Just a short question, can we use e.target.value to get value of input or we need to stick to $(e.target).val(). For now i did it with jquery but e.target.value is shorter - but not sure is it ok for all browsers.

Also I would like that someone with more states experience check

/**
 * Return the date for a given format string via Ajax.
 */
function system_date_time_lookup($form, $form_state) {
  $format = '';
  if (!empty($form_state['values']['date_format'])) {
    $format = t('Displayed as %date_format', array('%date_format' => format_date(REQUEST_TIME, 'custom', $form_state['values']['date_format'])));
  }
  // We are returning command and not directly string because if we return
  // string ajax api will prepend additional empty div element which will mess
  // our layout.
  $commands[] = ajax_command_replace('#edit-date-format-suffix', '<small id="edit-date-format-suffix">' . $format . '</small>');
  return array('#type' => 'ajax', '#commands' => $commands);
}

I mean it's ok that we use ajax_command_replace to avoid empty div problem but maybe there is more cleaner way?

ok for me.

Needs issue summary

Added issue summary and changed issue title.

Forgot to remove need issue summary tag.

This patch makes me happy :)

The #state thing is weird but that's what you got to do to make it work. We need a better ajax.js to fix that.

+++ b/core/includes/install.core.incundefined
@@ -1840,16 +1838,29 @@ function _install_configure_form($form, &$form_state, &$install_state) {
+    1 => array(
+      '#type' => 'checkbox',
+      '#title' => st('Check for updates automatically.'),
+      '#default_value' => TRUE,
+    ),
+    2 => array(

These numeric indicies are really hacky. Why are they needed? If the actual numbers are important, they should be comments.

Otherwise, this looks good.

it's kinda manually expanding checkboxes, that's what it'd become after form_process_checkboxes runs on it. Doesn't look great but looks valid to me.

Looking at it again It is missing the #return_value key for the individual checkboxes though.

These numeric indicies are really hacky. Why are they needed? If the actual numbers are important, they should be comments.

Comment in patch already saying it all

@@ -1840,16 +1838,31 @@ function _install_configure_form($form, &$form_state, &$install_state) {
     '#type' => 'fieldset',
     '#title' => st('Update notifications'),
     '#collapsible' => FALSE,
+    '#weight' => 15,
   );
+  // We are using container here and stupid 1,2 ids for checkboxes so our test
+  // bot is happy and do not break on install part.
   $form['update_notifications']['update_status_module'] = array(
-    '#type' => 'checkboxes',
-    '#options' => array(

So we are using numeric indicies to make testbot installer part not breaking. Original patch from 10 used normal text keys here but testbot was failing with them - I think that a problem is in testbot installation script - it needs that numeric keys here because original installer was using checkboxes with numeric keys - bad!
I agree this is ugly and hacky, but we need to change testbot installer part also then in order to make this more pretty.

Looking at it again It is missing the #return_value key for the individual checkboxes though

Added #return_value and rerolled patch against latest dev.

+++ b/core/includes/install.core.inc
@@ -1840,16 +1838,31 @@ function _install_configure_form($form, &$form_state, &$install_state) {
-    '#type' => 'checkboxes',
...
+    '#type' => 'container',
+    '#tree' => TRUE,
+    1 => array(
...
+    2 => array(

1) I'm not sure whether there isn't actually another issue for this already, but I'd really love to replace the numeric indexes with more meaningful keys; e.g., 'check' and 'notify'.

2) You do not have to switch to #type 'container' to achieve what you're doing. Instead, declare the #type 'checkboxes' as it was before, and then prepare the addition to the second as follows:

$form[...][2] = array(
  '#states' => array(...),
  '#description' => '...',
);

That works, because form_process_checkboxes() makes sure to merge any predefined properties into the expanded sub-elements. :)

+++ b/core/modules/system/system.admin.inc
@@ -2885,8 +2867,10 @@ function system_configure_date_formats_form($form, &$form_state, $dfid = 0) {
+      'progress' => array('type' => 'throbber', 'message' => NULL),

Aren't these the defaults?

+++ b/core/modules/system/system.admin.inc
@@ -2904,6 +2888,21 @@ function system_configure_date_formats_form($form, &$form_state, $dfid = 0) {
+  $commands[] = ajax_command_replace('#edit-date-format-suffix', '<small id="edit-date-format-suffix">' . $format . '</small>');

I wonder whether the code shouldn't use the actual HTML ID of the element as defined in $form instead of hard-coding #edit-date-format-suffix?

+++ b/core/modules/system/system.js
@@ -3,92 +3,32 @@
 Drupal.behaviors.copyFieldValue = {
   attach: function (context, settings) {
...
+    function updateTarget (e) {

Defining this as a function within the scope of .attach only is pretty confusing... I don't quite get how it is used/triggered...?

Lastly, some of the comments mention "ajax" and "api". The former should always be "Ajax", and when referring to Drupal's "Ajax framework", just use that term instead of "API" (always uppercase). :)

attached patch only addresses concerns regarding system.js, haven't touched the other concerns, so I'm leaving this as "needs work"
It felt silly abstracting a behavior that simple to a proper constructor, so I just abstracted the handler and put it on the behavior object.
Not sure if the internal mapping makes sense, but this would make it possible to call this behavior with settings that aren't present in Drupal.settings without asploding those settings.

All right, took another approach in the JS, hopefully that'll be clearer what's going on.

Changes the form stuff with sun suggestions. The #ajax throbber array is needed, otherwise there is a message, to remove the message and keep the spinner image, it needs to be like that (otherwise there is nothing showing up).

Can't use the ids from the ajax callback since drupal_html_id just gives crap results we can't use when you're in the ajax response.

Seems like a testbot issue, can install Drupal with and without JS activated on my laptop.

Sorry, my fault. The suggestion to change the key names in install_configure_form() does not work, because that requires changes to PIFR.

Re-rolled without that. Didn't test, but looks RTBC.

Small error in # 37 patch

@@ -1894,12 +1899,12 @@ function install_configure_form_submit($form, &$form_state) {
   variable_set('site_default_country', $form_state['values']['site_default_country']);
 
   // Enable update.module if this option was selected.
-  if ($form_state['values']['update_status_module'][1]) {
+  if ($form_state['values']['update_status_module']['check']) {
     module_enable(array('file', 'update'), FALSE);
 
     // Add the site maintenance account's email address to the list of
     // addresses to be notified when updates are available, if selected.
-    if ($form_state['values']['update_status_module'][2]) {
+    if ($form_state['values']['update_status_module']['notify']) {

instead of 'check' and 'notify' we should use old [1] and [2] keys. Beside that everything else looks good and I did full test of newest patch.

We talked at length in IRC about large refactoring patches like this, and their likelihood to break things in core. We decided to tag issues like this with "Needs JS testing" and then zendoodles and her Merry Band of Contributors will test these patches manually (for now) and with automated testing (later!).

Tagging so she can find it. :)

Test updated http://drupal.org/node/1777342 + reroll

reroll. Tests were added to the testing page http://drupal.org/node/1777342 and everything is working as intended.

Just looking at the code, it all seems pretty sane. I don't want to RTBC it, though, because I haven't actually tested it on a live Drupal install. It looks to be a very nice cleanup, though. Thanks!

Overall, this looks pretty good, but I have some concerns:

+++ b/core/modules/system/system.js
@@ -2,24 +2,34 @@
+ * This event handler will trigger a 'value:copy' event on all dependant fields.
...
+  $('#' + targetIds.join(', #')).trigger('value:copy', value);

1) Shouldn't the event name be namespaced in some way? "value:copy" looks like it has a high chance for running into unintentional name clashes with arbitrary other libraries that may run on the page.

2) It's the first time I see a colon in a event name. Why do we do that?

3) A more descriptive event name would be better; e.g., copyValueFromSource

+++ b/core/modules/system/system.js
@@ -28,66 +38,27 @@ Drupal.hideEmailAdministratorCheckbox = function () {
+      // We have to use body and not document because of the way jQuery events
+      // bubble up the DOM tree.
+      $('body').once('copy-field-values').on('value:copy', valueTargetCopyHandler);
...
+      $('body').removeOnce('copy-field-values').off('value:copy');

1) This comment leaves the reader in the dark for as to "why."

2) Why is this behavior completely ignoring context? That seems to be the root cause for why the code needs to explain internals about jQuery events, which it shouldn't have to.

3) Closely related to that, wouldn't it make sense to provide this behavior as a standalone file/library in /misc, so that modules which want the same behavior can simply load and apply it to their forms?

1, 2) have a look at states.js :)

3) if you have a better name than value:copy sure, I just wanted to keep the naming consistent with #states since it works the same way.

4) Because of jQuery. I don't think the comment should be used to explain jQuery internals. Sizzle can't access the document object so it can't be used to bind events and use the bubbling propriety of the events. I don't remeber the details right now but I tried with document, it didn't work and looked up how events were managed in jQuery to find out what I wanted to do wouldn't work. states.js is using document but it's using events in a rather convoluted way.

5) It's ignoring the context because we're working with IDs and because we're binding only one event since we don't need more than one event listener for all this.

6) I'm not sure that's wanted. It's pretty short and once you get how it works it's pretty simple. We can make things short and simple here since we can rely on the fact that we're using IDs all over the place. It's not the case for #states and would introduce more code for the same result.

Back to RTBC, all it was missing was tests in #39, they've been added and patch works.

Why are those event handlers not being defined as Drupal.behaviors.copyFieldValue.valueSourceBlurHandler? That way, it would be rather easy for 3rd party scripts to override it if they wanted to, right now, they would either need to override the entire behavior, or just swap out the file altogether...

the file is really small now, so if you need to override one part it's just easier to override the whole thing. Loading dead code shouldn't be our aim as an override mechanism. I'd rather have everything as small as possible and separated in it's own file than doing monkey patching of big files.

True, but having it on the behavior object wouldn't increase the file size by much, would it? just the extra bytes for indentation afaict...

feel free to reroll, The main problem with the issue is that Drupal doesn't use it's own API, that's small stuff I don't care much either way :p

I don't really care that much, that's why I didn't change the status but I can imagine that when some1 is debugging this, it's nice to have a reference-able function that isn't hidden inside a closure

works for me, thanks.

adding a few things, hang on for a new patch.

Some dependency things :)

tests added, removing tag

oh, right :P

JS only patch, testbot failure irrelevant.

yeah, I don't rly get how it could fail before that page

stepped through the installer to make sure and it works as expected... e-mail was properly copied over

Looks like good clean-up. Committed/pushed to 8.x.

Issue summary points that we need backport

This would be an API change — and worse, in some code that isn't tested well.