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.

CommentFileSizeAuthor
#76 ajax-1987602-76.patch29.52 KBeffulgentsia
#76 interdiff.txt4.82 KBeffulgentsia
#74 ajax-1987602-74.patch29.25 KBtim.plunkett
#71 interdiff.txt2.01 KBjuampynr
#71 drupal-form-ajax-callbacks-to-controller-1987602-71.patch29.12 KBjuampynr
#70 interdiff.txt4.92 KBjuampynr
#70 drupal-form-ajax-callbacks-to-controller-1987602-70.patch28.42 KBjuampynr
#67 drupal-form-ajax-callbacks-to-controller-1987602-67.patch24.51 KBjuampynr
#67 interdiff.txt1.62 KBjuampynr
#62 drupal-form-ajax-callbacks-to-controller-1987602-61.patch22.89 KBjuampynr
#62 interdiff.txt946 bytesjuampynr
#60 drupal-form-ajax-callbacks-to-controller-1987602-60.patch23.02 KBjuampynr
#60 interdiff.txt1.79 KBjuampynr
#59 form-ajax-callbacks-to-controller-1987602-59.patch22.9 KBjuampynr
#56 form-ajax-callbacks-to-controller-1987602-56.patch26.74 KBpwieck
#54 ajax_form-1987602-54.patch26.74 KBdawehner
#47 form-ajax-callbacks-to-controller-1987602-47.patch26.74 KBpwieck
#42 form-ajax-callbacks-to-controller-1987602-42.patch26.74 KBglennpratt
#30 form-ajax-callbacks-to-controller-1987602-30.patch26.5 KBglennpratt
#22 form-ajax-callbacks-to-controller-1987602-22.patch26.51 KBglennpratt
#20 1987602-form-ajax-callbacks-to-controller-20.patch26.49 KBvijaycs85
#14 form-ajax-callbacks-to-controller-1987602-13.patch26.46 KBglennpratt
#11 form-ajax-callbacks-to-controller-1987602-11.patch26.46 KBglennpratt
#8 form-ajax-callbacks-to-controller-1987602-8.patch26.04 KBglennpratt
#6 form-ajax-callbacks-to-controller-1987602-5.patch25.94 KBglennpratt
#4 form-ajax-callbacks-to-controller-1987602-4.patch25.93 KBglennpratt
#3 form-ajax-callbacks-to-controller-1987602-3.patch25.92 KBglennpratt
#2 system-ajax-controller-1987602-2.patch5.13 KBglennpratt
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

glennpratt’s picture

Assigned: Unassigned » glennpratt
glennpratt’s picture

Status: Active » Needs review
FileSize
5.13 KB

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

glennpratt’s picture

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.

glennpratt’s picture

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.

glennpratt’s picture

Whoops, silly namespace mistake.

glennpratt’s picture

Status: Needs work » Needs review
glennpratt’s picture

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

glennpratt’s picture

EDIT double submit...

tstoeckler’s picture

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

glennpratt’s picture

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

glennpratt’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

glennpratt’s picture

Status: Needs work » Needs review
FileSize
26.46 KB

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.

glennpratt’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

glennpratt’s picture

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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
26.49 KB

Re-rolling...

Status: Needs review » Needs work

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

glennpratt’s picture

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.

glennpratt’s picture

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.

glennpratt’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI-conversion
tstoeckler’s picture

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

dawehner’s picture

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?

glennpratt’s picture

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.

dawehner’s picture

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.

glennpratt’s picture

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.

dawehner’s picture

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.

glennpratt’s picture

dawehner’s picture

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I should concentrate more.

thedavidmeister’s picture

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.

glennpratt’s picture

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?

tstoeckler’s picture

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.

thedavidmeister’s picture

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

alexpott’s picture

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
tstoeckler’s picture

Sure, we could certainly do #38.

SoumyaDas’s picture

Assigned: glennpratt » SoumyaDas

Hi,
I have started working on "Needs reroll"

Thanks,
Soumya Sona Das

glennpratt’s picture

Status: Needs work » Needs review
FileSize
26.74 KB

Got impatient ssdas, sorry.

dawehner’s picture

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.

dawehner’s picture

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

Readd the tags and remove the assignment.

alexpott’s picture

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
pwieck’s picture

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

pwieck’s picture

Status: Needs work » Needs review
FileSize
26.74 KB

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.

pwieck’s picture

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.

thedavidmeister’s picture

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

pwieck’s picture

Good for review!

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Let's do this, finally. :-)

star-szr’s picture

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
dawehner’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
26.74 KB

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.

pwieck’s picture

Status: Needs work » Needs review
FileSize
26.74 KB

Re-rolled again to current head:-)

glennpratt’s picture

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

dawehner’s picture

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

juampynr’s picture

Status: Needs review » Needs work
FileSize
22.9 KB

Re-rolling.

juampynr’s picture

Status: Needs work » Needs review
FileSize
1.79 KB
23.02 KB

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.

juampynr’s picture

Status: Needs work » Needs review
FileSize
946 bytes
22.89 KB

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

dawehner’s picture

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

That is looking great now.

juampynr’s picture

Assigned: Unassigned » juampynr

Assigning to myself.

YesCT’s picture

Issue tags: +RTBC July 1

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

alexpott’s picture

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

juampynr’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.62 KB
24.51 KB

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

The test is looking great.

effulgentsia’s picture

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.

juampynr’s picture

Status: Needs work » Needs review
FileSize
28.42 KB
4.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.

juampynr’s picture

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.

juampynr’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI-conversion, +RTBC July 1
tim.plunkett’s picture

juampynr’s picture

Thanks for re-rolling @tim.plunkett.

I still need feedback from @effulgentsia at #71.

effulgentsia’s picture

FileSize
4.82 KB
29.52 KB

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.

juampynr’s picture

Status: Needs review » Reviewed & tested by the community

This is ready to go!

tim.plunkett’s picture

+1

xjm’s picture

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

webchick’s picture

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!

vijaycs85’s picture

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

quietone’s picture

Issue summary: View changes

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