Convert ajax_form_callback() to a new style controller

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

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.

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

Whoops, silly namespace mistake.

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

EDIT double submit...

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

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

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

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

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

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.

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

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.

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.

I left the hook_menu entries because of #1954892: Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system.

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

I should concentrate more.

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?

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.

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.

Hi,
I have started working on "Needs reroll"

Thanks,
Soumya Sona Das

Got impatient ssdas, sorry.

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

Readd the tags and remove the assignment.

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.

rerolled

#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!

Let's do this, finally. :-)

Needs another reroll unfortunately.

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

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

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)

Re-rolling.

Improved docblocks and fixed route patterns.

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

That is looking great now.

Assigning to myself.

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

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

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

The test is looking great.

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.

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.

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

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

Rerolled for #1999370: Use Symfony Request for file module and #2009014: Replace theme() with drupal_render() in file module.

This blocks #1959574: Remove the deprecated Drupal 7 Ajax API

Thanks for re-rolling @tim.plunkett.

I still need feedback from @effulgentsia at #71.

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.

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?

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!

Closed #1954882: Convert system/ajax to a route as a duplicate and adding credit.