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.

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?

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.

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.

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

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.

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.

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

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.