Part of #1971384: [META] Convert page callbacks to controllers

For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.

Files: 
CommentFileSizeAuthor
#76 ajax-1987602-76.patch29.52 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 57,645 pass(es).
[ View ]
#76 interdiff.txt4.82 KBeffulgentsia
#74 ajax-1987602-74.patch29.25 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,204 pass(es).
[ View ]
#71 interdiff.txt2.01 KBjuampy
#71 drupal-form-ajax-callbacks-to-controller-1987602-71.patch29.12 KBjuampy
PASSED: [[SimpleTest]]: [MySQL] 56,872 pass(es).
[ View ]
#70 interdiff.txt4.92 KBjuampy
#70 drupal-form-ajax-callbacks-to-controller-1987602-70.patch28.42 KBjuampy
PASSED: [[SimpleTest]]: [MySQL] 57,161 pass(es).
[ View ]
#67 drupal-form-ajax-callbacks-to-controller-1987602-67.patch24.51 KBjuampy
PASSED: [[SimpleTest]]: [MySQL] 56,919 pass(es).
[ View ]
#67 interdiff.txt1.62 KBjuampy
#62 drupal-form-ajax-callbacks-to-controller-1987602-61.patch22.89 KBjuampy
PASSED: [[SimpleTest]]: [MySQL] 56,388 pass(es).
[ View ]
#62 interdiff.txt946 bytesjuampy
#60 drupal-form-ajax-callbacks-to-controller-1987602-60.patch23.02 KBjuampy
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#60 interdiff.txt1.79 KBjuampy
#59 form-ajax-callbacks-to-controller-1987602-59.patch22.9 KBjuampy
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#56 form-ajax-callbacks-to-controller-1987602-56.patch26.74 KBpwieck
PASSED: [[SimpleTest]]: [MySQL] 57,862 pass(es).
[ View ]
#54 ajax_form-1987602-54.patch26.74 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch ajax_form-1987602-54.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#47 form-ajax-callbacks-to-controller-1987602-47.patch26.74 KBpwieck
PASSED: [[SimpleTest]]: [MySQL] 58,142 pass(es).
[ View ]
#42 form-ajax-callbacks-to-controller-1987602-42.patch26.74 KBglennpratt
PASSED: [[SimpleTest]]: [MySQL] 57,297 pass(es).
[ View ]
#30 form-ajax-callbacks-to-controller-1987602-30.patch26.5 KBglennpratt
PASSED: [[SimpleTest]]: [MySQL] 57,053 pass(es).
[ View ]
#22 form-ajax-callbacks-to-controller-1987602-22.patch26.51 KBglennpratt
PASSED: [[SimpleTest]]: [MySQL] 56,805 pass(es).
[ View ]
#20 1987602-form-ajax-callbacks-to-controller-20.patch26.49 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 57,280 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#14 form-ajax-callbacks-to-controller-1987602-13.patch26.46 KBglennpratt
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form-ajax-callbacks-to-controller-1987602-13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#11 form-ajax-callbacks-to-controller-1987602-11.patch26.46 KBglennpratt
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]
#8 form-ajax-callbacks-to-controller-1987602-8.patch26.04 KBglennpratt
PASSED: [[SimpleTest]]: [MySQL] 55,928 pass(es).
[ View ]
#6 form-ajax-callbacks-to-controller-1987602-5.patch25.94 KBglennpratt
PASSED: [[SimpleTest]]: [MySQL] 55,839 pass(es).
[ View ]
#4 form-ajax-callbacks-to-controller-1987602-4.patch25.93 KBglennpratt
FAILED: [[SimpleTest]]: [MySQL] 56,927 pass(es), 78 fail(s), and 20 exception(s).
[ View ]
#3 form-ajax-callbacks-to-controller-1987602-3.patch25.92 KBglennpratt
PASSED: [[SimpleTest]]: [MySQL] 56,524 pass(es).
[ View ]
#2 system-ajax-controller-1987602-2.patch5.13 KBglennpratt
PASSED: [[SimpleTest]]: [MySQL] 55,836 pass(es).
[ View ]

Comments

Assigned:Unassigned» glennpratt

Status:Active» Needs review
StatusFileSize
new5.13 KB
PASSED: [[SimpleTest]]: [MySQL] 55,836 pass(es).
[ View ]

Abusing the test bot with initial patch. Several TODOs on naming and refactoring for Request object.

StatusFileSize
new25.92 KB
PASSED: [[SimpleTest]]: [MySQL] 56,524 pass(es).
[ View ]

I merged in #1978892: Convert file_ajax_upload() to a Controller because of a common dependence on ajax_get_form() helper. Then I merged in #1978894: Convert file_ajax_progress() to a Controller because it is a closely related route (upload and progress).

Let me know if I've gone overboard moving ajax_get_form() helper into the controller.

StatusFileSize
new25.93 KB
FAILED: [[SimpleTest]]: [MySQL] 56,927 pass(es), 78 fail(s), and 20 exception(s).
[ View ]

Removed unused use statement and moved system.ajax into Controller subdir.

Status:Needs review» Needs work

The last submitted patch, form-ajax-callbacks-to-controller-1987602-4.patch, failed testing.

StatusFileSize
new25.94 KB
PASSED: [[SimpleTest]]: [MySQL] 55,839 pass(es).
[ View ]

Whoops, silly namespace mistake.

Status:Needs work» Needs review

StatusFileSize
new26.04 KB
PASSED: [[SimpleTest]]: [MySQL] 55,928 pass(es).
[ View ]

pdrake asked me to remove the file includes since they don't do anything anymore according to the documentation.

EDIT double submit...

Status:Needs review» Needs work

This looks great overall!!! I have some minor suggestions for improvement:

+++ b/core/includes/ajax.inc
@@ -21,9 +21,9 @@
+ * Drupal\system\FormAjaxController::content() and a defined #ajax['callback']

Here and elsewhere: when specifying a fully namespaced class one should start with a leading backslash, i.e. \Drupal\system\...

+++ b/core/includes/ajax.inc
@@ -36,16 +36,16 @@
+ *   - Controller 'system/ajax', Drupal\system\FormAjaxController::content(),

I think this should be: "The controller for the route 'system/ajax', \Drupal\...content(), ..."

I'm not sure if, in this case, it should be /system/ajax (with a leading slash), as that is the convention Symfony uses, and by extension Drupal for its route declaration. In the context of paths (not routes) we generally don't add the leading slash.

+++ b/core/includes/form.inc
@@ -1274,9 +1275,10 @@ function drupal_validate_form($form_id, &$form, &$form_state) {
+ *   set, for instance, by Drupal\system\FormAjaxController::getForm() prevent

*to* prevent

+++ b/core/modules/file/lib/Drupal/file/Controller/FileWidgetAjaxController.php
@@ -0,0 +1,114 @@
+   * @param Symfony\Component\HttpFoundation\Request $request

In @param and @return as well, the full namespace should start with a backslash.

+++ b/core/modules/file/lib/Drupal/file/Controller/FileWidgetAjaxController.php
@@ -0,0 +1,114 @@
+    if (empty($form_build_id)) {

It seems previously the form build ID was compared with $_POST. I don't know why this was dropped.

+++ b/core/modules/file/lib/Drupal/file/Controller/FileWidgetAjaxController.php
@@ -0,0 +1,114 @@
+    $current_element = $form;
+    foreach ($form_parents as $parent) {
+      $current_element = $current_element[$parent];
+    }

I realize this is identical to the previous code, but anyway: Couldn't this use \Drupal\Component\Utility\NestedArray::getValue()?

+++ b/core/modules/file/lib/Drupal/file/Controller/FileWidgetAjaxController.php
@@ -0,0 +1,114 @@
+    // Retrieve the element to be rendered.
+    foreach ($form_parents as $parent) {
+      $form = $form[$parent];
+    }

Again, I think this could also use NestedArray.
Again, it's fine if we leave optimizing this function to a follow-up.

+++ b/core/modules/system/system.routing.yml
@@ -1,3 +1,9 @@
+  pattern: 'system/ajax'

As can be seen in the patch context the pattern should be '/system/ajax', i.e. start with a slash.

+++ b/core/modules/file/file.routing.yml
@@ -0,0 +1,12 @@
+file_ajax_upload:
...
+file_ajax_progress:

I think we usually use a dot (.) for namespacing of route names, i.e. I would suggest file.ajax.upload and file.upload.progress

StatusFileSize
new26.46 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

Thanks for the review! I think I addressed each item in this patch.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, form-ajax-callbacks-to-controller-1987602-11.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new26.46 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form-ajax-callbacks-to-controller-1987602-13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Ahh, forgot I didn't review all the class names earlier, this should be better.

Status:Needs review» Needs work
Issue tags:-WSCCI-conversion

The last submitted patch, form-ajax-callbacks-to-controller-1987602-13.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, form-ajax-callbacks-to-controller-1987602-13.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+WSCCI-conversion

The last submitted patch, form-ajax-callbacks-to-controller-1987602-13.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new26.49 KB
FAILED: [[SimpleTest]]: [MySQL] 57,280 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Re-rolling...

Status:Needs review» Needs work

The last submitted patch, 1987602-form-ajax-callbacks-to-controller-20.patch, failed testing.

StatusFileSize
new26.51 KB
PASSED: [[SimpleTest]]: [MySQL] 56,805 pass(es).
[ View ]

Thanks for the re-roll vijaycs85. Looks like the fix from #1792032: File validation error message not removed after subsequent upload of valid file was lost.

New patch.

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:-WSCCI-conversion

The last submitted patch, form-ajax-callbacks-to-controller-1987602-22.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+WSCCI-conversion

Looks very good to me. I don't feel smart enough, however, to RTBC.

Oh #1987712: Convert file_download() to a new style controller there was a similar problem, as the new route system does not allow arbitrary parameters. The workaround was to write a inbound alter subscriber which changes the url before the route system comes into the game. Maybe this would be possible/should be done here as well?

I didn't like that option since it's not really required here, the URL isn't a technical requirement like it might be with image styles.

glennpratt told me that nice url are not a requirement here, so no need for this extra stuff.

Great stuff so far!
+++ b/core/modules/file/lib/Drupal/file/Controller/FileWidgetAjaxController.phpundefined
@@ -0,0 +1,112 @@
+ * Definition of Drupal\file\FileWidgetAjaxController.

Should be "Contains \."

+++ b/core/modules/file/lib/Drupal/file/Controller/FileWidgetAjaxController.phpundefined
@@ -0,0 +1,112 @@
+   * Handle form AHAH request.

Wasn't ahah a concept of just drupal 6. We could update the comment here.

+++ b/core/modules/file/lib/Drupal/file/Controller/FileWidgetAjaxController.phpundefined
@@ -0,0 +1,112 @@
+        'message' => t('Starting upload...'),
+        'percentage' => -1,

Wrong indentation.

+++ b/core/modules/file/lib/Drupal/file/Controller/FileWidgetAjaxController.phpundefined
@@ -0,0 +1,112 @@
+    $implementation = file_progress_implementation();
+    if ($implementation == 'uploadprogress') {
+      $status = uploadprogress_get_info($key);
+      if (isset($status['bytes_uploaded']) && !empty($status['bytes_total'])) {
+        $progress['message'] = t('Uploading... (@current of @total)', array('@current' => format_size($status['bytes_uploaded']), '@total' => format_size($status['bytes_total'])));
+        $progress['percentage'] = round(100 * $status['bytes_uploaded'] / $status['bytes_total']);
+      }
+    }
+    elseif ($implementation == 'apc') {
+      $status = apc_fetch('upload_' . $key);
+      if (isset($status['current']) && !empty($status['total'])) {
+        $progress['message'] = t('Uploading... (@current of @total)', array('@current' => format_size($status['current']), '@total' => format_size($status['total'])));
+        $progress['percentage'] = round(100 * $status['current'] / $status['total']);
+      }

That's a classical examle what should be done in two separate objects ... What about open a follow up?

+++ b/core/modules/system/lib/Drupal/system/Controller/FormAjaxController.phpundefined
@@ -0,0 +1,98 @@
+ * Definition of Drupal\system\FormAjaxController.

Contains

+++ b/core/modules/system/lib/Drupal/system/Controller/FormAjaxController.phpundefined
@@ -0,0 +1,98 @@
+   * Handle form AHAH request.

also still refer to old stuff.

+++ b/core/modules/system/lib/Drupal/system/Controller/FormAjaxController.phpundefined
@@ -0,0 +1,98 @@
+  public function getForm(Request $request) {

Can't this one be protected? It's just used in the sub class.

StatusFileSize
new26.5 KB
PASSED: [[SimpleTest]]: [MySQL] 57,053 pass(es).
[ View ]

Does seem like there should be a followup for file_progress_implementation and the if statements to make them OO and pluggable... not sure what it should be called or if that fall under and existing initiative.

Addressed the other comments.

Uploading a single file works fine, just to be sure.

+++ b/core/modules/file/file.moduleundefined
@@ -46,14 +46,12 @@ function file_menu() {
   $items['file/ajax'] = array(
-    'page callback' => 'file_ajax_upload',
-    'access arguments' => array('access content'),
+    'route_name' => 'file.ajax.upload',
     'theme callback' => 'ajax_base_page_theme',
     'type' => MENU_CALLBACK,
   );
   $items['file/progress'] = array(
-    'page callback' => 'file_ajax_progress',
-    'access arguments' => array('access content'),
+    'route name' => 'file.ajax.progress',
     'theme callback' => 'ajax_base_page_theme',
     'type' => MENU_CALLBACK,
   );
@@ -724,100 +722,6 @@ function file_cron() {

All callbacks can be skipped.

+++ b/core/modules/file/lib/Drupal/file/Controller/FileWidgetAjaxController.phpundefined
@@ -0,0 +1,112 @@
+   * @param $key

@param string $key

+++ b/core/modules/system/lib/Drupal/system/Controller/FormAjaxController.phpundefined
@@ -0,0 +1,98 @@
+   * @return

@return array

+++ b/core/modules/system/system.moduleundefined
@@ -633,12 +633,9 @@ function system_menu() {
   $items['system/ajax'] = array(
     'title' => 'AHAH callback',
-    'page callback' => 'ajax_form_callback',
-    'access callback' => TRUE,
+    'route_name' => 'system.ajax',
     'theme callback' => 'ajax_base_page_theme',
     'type' => MENU_CALLBACK,
-    'file path' => 'core/includes',
-    'file' => 'form.inc',

Any reason why this MENU_CALLBACK is left? It's not really needed anymore.

Right, the first two parts though don't have a 'theme callback'.

Status:Needs review» Reviewed & tested by the community

I should concentrate more.

Status:Reviewed & tested by the community» Needs work

Minor but

+    $form['#prefix'] .= theme('status_messages');

We're trying to remove theme() calls from core #2006152: [meta] Don't call theme() directly anywhere outside drupal_render() so please don't introduce a new one.

I'm not introducing a new one really, just moving it. The relevant issue doesn't seem to have any progress #2007124: Replace theme() with drupal_render() in authorize.php.

Can you provide the correct code?

Status:Needs work» Reviewed & tested by the community

Yeah, let's postpone that for now. I'm not sure it's such a trivial replacement in this case, as we're adding the themed output directly to $form['#prefix']. At least I'm not entirely sure off-hand how.

$status_messages = array('#theme' => 'status_messages');
$form['#prefix'] .= drupal_render($status_messages);

Should do it, but fair enough that the other issue should pick it up.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Needs a reroll

curl https://drupal.org/files/form-ajax-callbacks-to-controller-1987602-30.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 27132  100 27132    0     0  20748      0  0:00:01  0:00:01 --:--:-- 23904
error: patch failed: core/includes/ajax.inc:300
error: core/includes/ajax.inc: patch does not apply

Sure, we could certainly do #38.

Assigned:glennpratt» ssdas

Hi,
I have started working on "Needs reroll"

Thanks,
Soumya Sona Das

Status:Needs work» Needs review
StatusFileSize
new26.74 KB
PASSED: [[SimpleTest]]: [MySQL] 57,297 pass(es).
[ View ]

Got impatient ssdas, sorry.

Status:Needs review» Reviewed & tested by the community
Issue tags:-WSCCI-conversion

+++ b/core/modules/file/lib/Drupal/file/Controller/FileWidgetAjaxController.phpundefined
@@ -0,0 +1,116 @@
+   * @return \Symfony\Component\HttpFoundation\Response
+   *   A Symfony response object.

... We could specify even with more detail: JsonResponse.

Assigned:ssdas» Unassigned
Issue tags:+WSCCI-conversion

Readd the tags and remove the assignment.

Status:Reviewed & tested by the community» Needs work

Needs a reroll

curl https://drupal.org/files/form-ajax-callbacks-to-controller-1987602-42.patch | git apply --check
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 27382  100 27382    0     0  14794      0  0:00:01  0:00:01 --:--:-- 16625
error: patch failed: core/modules/file/file.field.inc:486
error: core/modules/file/file.field.inc: patch does not apply

working on reroll of #42 disregarding #43 because I don't have enough info.

Status:Needs work» Needs review
StatusFileSize
new26.74 KB
PASSED: [[SimpleTest]]: [MySQL] 58,142 pass(es).
[ View ]

rerolled

Status:Needs review» Needs work
Issue tags:-Needs reroll, -WSCCI-conversion

The last submitted patch, form-ajax-callbacks-to-controller-1987602-47.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Needs reroll, +WSCCI-conversion

#47: form-ajax-callbacks-to-controller-1987602-47.patch queued for re-testing.

Why are many of my rerolls failing like this? Sometimes re-test passes and some times not. Seem like a bug.

it does seem like a bug. I can't even see what the fatal error is in the testbot logs.

Good for review!

Status:Needs review» Reviewed & tested by the community

Let's do this, finally. :-)

Status:Reviewed & tested by the community» Needs work

Needs another reroll unfortunately.

error: patch failed: core/includes/form.inc:1274
error: core/includes/form.inc: patch does not apply

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new26.74 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch ajax_form-1987602-54.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

patch -p1 < form-ajax-callbacks-to-controller-1987602-47.patch
patching file core/includes/ajax.inc
patching file core/includes/form.inc
patching file core/modules/field/field.form.inc
patching file core/modules/file/file.field.inc
Hunk #1 succeeded at 475 (offset -5 lines).
patching file core/modules/file/file.module
patching file core/modules/file/file.routing.yml
patching file core/modules/file/lib/Drupal/file/Controller/FileWidgetAjaxController.php
patching file core/modules/system/lib/Drupal/system/Controller/FormAjaxController.php
patching file core/modules/system/system.module
patching file core/modules/system/system.routing.yml

Status:Reviewed & tested by the community» Needs work

The last submitted patch, ajax_form-1987602-54.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new26.74 KB
PASSED: [[SimpleTest]]: [MySQL] 57,862 pass(es).
[ View ]

Re-rolled again to current head:-)

Just about cross posted with the exact same patch...

Here's my Git branch for people who live in the future.

https://github.com/glennpratt/drupal/tree/1987602-system-ajax-controller

+++ b/core/modules/file/lib/Drupal/file/Controller/FileWidgetAjaxController.phpundefined
@@ -0,0 +1,116 @@
+   * @return \Symfony\Component\HttpFoundation\Response
+   *   A Symfony response object.
...
+   * @return \Symfony\Component\HttpFoundation\Response
+   *   A Symfony response object.

Actually it is always an ajax|json response. ... It is possible to find a more helpful comment here? (sorry)

Status:Needs review» Needs work
StatusFileSize
new22.9 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Re-rolling.

Status:Needs work» Needs review
StatusFileSize
new1.79 KB
new23.02 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Improved docblocks and fixed route patterns.

Status:Needs review» Needs work

The last submitted patch, drupal-form-ajax-callbacks-to-controller-1987602-60.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new946 bytes
new22.89 KB
PASSED: [[SimpleTest]]: [MySQL] 56,388 pass(es).
[ View ]

Added missing blank lines just before the closing parenthesis of the two new classes.

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs reroll

That is looking great now.

Assigned:Unassigned» juampy

Assigning to myself.

Issue tags:+RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests

+++ b/core/includes/ajax.incundefined
@@ -302,88 +303,6 @@ function ajax_render($commands = array()) {
-function ajax_form_callback() {
-  list($form, $form_state) = ajax_get_form();
-  drupal_process_form($form['#form_id'], $form, $form_state);
-
-  // We need to return the part of the form (or some other content) that needs
-  // to be re-rendered so the browser can update the page with changed content.
-  // Since this is the generic menu callback used by many Ajax elements, it is
-  // up to the #ajax['callback'] function of the element (may or may not be a
-  // button) that triggered the Ajax request to determine what needs to be
-  // rendered.
-  if (!empty($form_state['triggering_element'])) {
-    $callback = $form_state['triggering_element']['#ajax']['callback'];
-  }
-  if (!empty($callback) && is_callable($callback)) {
-    return call_user_func_array($callback, array(&$form, &$form_state));
-  }
-}
+++ b/core/modules/system/lib/Drupal/system/Controller/FormAjaxController.phpundefined
@@ -0,0 +1,100 @@
+  public function content(Request $request) {
+    list($form, $form_state) = $this->getForm($request);
+    drupal_process_form($form['#form_id'], $form, $form_state);
+
+    // We need to return the part of the form (or some other content) that needs
+    // to be re-rendered so the browser can update the page with changed content.
+    // Since this is the generic menu callback used by many Ajax elements, it is
+    // up to the #ajax['callback'] function of the element (may or may not be a
+    // button) that triggered the Ajax request to determine what needs to be
+    // rendered.
+    if (!empty($form_state['triggering_element'])) {
+      $callback = $form_state['triggering_element']['#ajax']['callback'];
+    }
+    if (empty($callback) || !is_callable($callback)) {
+      throw new HttpException(500, t('Internal Server Error'));
+    }
+    return call_user_func_array($callback, array(&$form, &$form_state));
+  }

Looks like we've changed ajax_form_callback to throw a http exception if we don't have a callback. Whilst this change does make sense and should make Drupal less brittle I think it means we need a test so that we don't accidentally remove this behaviour in the future...

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new1.62 KB
new24.51 KB
PASSED: [[SimpleTest]]: [MySQL] 56,919 pass(es).
[ View ]

Sure, I added a test to verify that an AJAX form element without callback throws a 500.

Status:Needs review» Reviewed & tested by the community

The test is looking great.

Status:Reviewed & tested by the community» Needs work

This looks really good overall. Just some minor feedback. Sorry for downgrading from RTBC, but I think it makes sense to get these quick fixes in with the initial patch.

+++ b/core/modules/file/file.module
@@ -46,14 +46,12 @@ function file_menu() {
-    'page callback' => 'file_ajax_upload',
...
-    'page callback' => 'file_ajax_progress',

Let's remove the functions themselves as well then.

+++ b/core/modules/file/lib/Drupal/file/Controller/FileWidgetAjaxController.php
@@ -0,0 +1,117 @@
+   * Handle form AJAX request.
...
+   * Handle AJAX upload progress request.
...
+++ b/core/modules/system/lib/Drupal/system/Controller/FormAjaxController.php
@@ -0,0 +1,100 @@
+   * Handle form AJAX request.

These controller methods could all use a more informative 1 line summary, a little more like the functions they replace. Also, the initial verb should end in an "s" (e.g., "Handles").

+++ b/core/modules/system/tests/modules/ajax_forms_test/ajax_forms_test.module
@@ -72,6 +72,14 @@ function ajax_forms_test_simple_form($form, &$form_state) {
+    '#ajax' => array('callback' => ''),

This test is good, but it only catches 1 of 3 situations. The other 2 are '#ajax' => array('path' => 'system/ajax') and '#ajax' => array('callback' => 'SOME_FUNCTION_THAT_DOES_NOT_EXIST'), and I would argue that these 2 are more likely, so let's include them as well.

Status:Needs work» Needs review
StatusFileSize
new28.42 KB
PASSED: [[SimpleTest]]: [MySQL] 57,161 pass(es).
[ View ]
new4.92 KB

Great feedback, thanks!

This patch addresses all suggestions except the ones for the test, which I will work on in the next patch. In the meantime I want all tests to be run over these changes (see interdiff) and @effulgentsia to confirm the updated method descriptions based on his suggestion.

Now I will review those missing test scenarios and update the test accordingly.

StatusFileSize
new29.12 KB
PASSED: [[SimpleTest]]: [MySQL] 56,872 pass(es).
[ View ]
new2.01 KB

Here I am adding an extra assertion to test '#ajax' => array('callback' => 'SOME_FUNCTION_THAT_DOES_NOT_EXIST').

@effulgentsia, could you give me more background on scenarios of '#ajax' => array('callback' => 'SOME_FUNCTION_THAT_DOES_NOT_EXIST')? I do not know how to test it nor why is it related with FormAjaxController->content().

Status:Needs review» Needs work
Issue tags:-WSCCI-conversion, -RTBC July 1

The last submitted patch, drupal-form-ajax-callbacks-to-controller-1987602-71.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+WSCCI-conversion, +RTBC July 1

Issue tags:+blocker
StatusFileSize
new29.25 KB
PASSED: [[SimpleTest]]: [MySQL] 57,204 pass(es).
[ View ]

Thanks for re-rolling @tim.plunkett.

I still need feedback from @effulgentsia at #71.

StatusFileSize
new4.82 KB
new29.52 KB
PASSED: [[SimpleTest]]: [MySQL] 57,645 pass(es).
[ View ]

Sorry for the delay in responding.

I do not know how to test it nor why is it related with FormAjaxController->content().

It's related, because that's where we throw the error within this if statement: if (empty($callback) || !is_callable($callback)) {, so if we're adding test coverage here for that, then let's add all the relevant conditions that can trigger it.

The nonexistent function added in #71 looks good. This just also adds the NULL case, and now that there's 3, puts them into a loop.

This also cleans up a couple docs.

I have no other nits to pick, so if you're happy with these changes, please RTBC.

Status:Needs review» Reviewed & tested by the community

This is ready to go!

+1

+++ b/core/modules/system/lib/Drupal/system/Controller/FormAjaxController.phpundefined
@@ -0,0 +1,105 @@
+      throw new HttpException(500, t('Internal Server Error'));

Don't we not translate exception messages?

Status:Reviewed & tested by the community» Fixed
Issue tags:+Needs followup

Looks good to me; xjm is right that we shouldn't be translating those messages, but Alex found a few other places where that's happening, so let's get a follow-up filed to fix it everywhere in core.

Committed and pushed to 8.x. Thanks!

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