#1938980: Fix Ajax system; the last remnants of the old API must be swept away went into various troubles with using '#type' => 'ajax', so let's replace all of the places to use AjaxResponse directly.

CommentFileSizeAuthor
#14 interdiff.txt12.62 KBmkadin
#14 drupal_remove_old_ajax_1957590_14.patch47.32 KBmkadin
#12 drupal_remove_old_ajax_1957590_11.patch43.2 KBmkadin
#8 drupal-1957590-8.patch20.68 KBdawehner
#8 interdiff.txt500 bytesdawehner
#6 drupal-1957590-6.patch20.7 KBdawehner
#6 interdiff.txt15.71 KBdawehner
#1 drupal-1957590-1.patch12.66 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Active » Needs review
FileSize
drupal-1957590-1.patch12.66 KB

Let's see what the testbot tells us.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented 
+++ b/core/modules/system/tests/modules/ajax_forms_test/ajax_forms_test.moduleundefined
@@ -5,6 +5,21 @@
+use Drupal\Core\Ajax\AfterCommand;
+use Drupal\Core\Ajax\AjaxResponse;
+use Drupal\Core\Ajax\AlertCommand;
+use Drupal\Core\Ajax\AppendCommand;
+use Drupal\Core\Ajax\BeforeCommand;
+use Drupal\Core\Ajax\ChangedCommand;
+use Drupal\Core\Ajax\CssCommand;
+use Drupal\Core\Ajax\DataCommand;
+use Drupal\Core\Ajax\HtmlCommand;
+use Drupal\Core\Ajax\InsertCommand;
+use Drupal\Core\Ajax\InvokeCommand;
+use Drupal\Core\Ajax\PrependCommand;
+use Drupal\Core\Ajax\RemoveCommand;
+use Drupal\Core\Ajax\RestripeCommand;
+use Drupal\Core\Ajax\SettingsCommand;

This is one of the times you should probably just have use Drupal\Core\Ajax; and then have new Ajax\AjaxResponse(); inline. Drupal\views_ui\Form\Ajax\ViewsFormBase does this with Drupal\views\Ajax\*

Status: Needs review » Needs work

The last submitted patch, drupal-1957590-1.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Let's get rid of ajax_command_*() here. I think it's right way to make sure that nothing is broken

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented

FWIW, +1 to removing #type => 'ajax'. That was a D7 hack/abuse of render arrays, because we had no alternative. In D8, AjaxResponse is the better API.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
interdiff.txt15.71 KB
drupal-1957590-6.patch20.7 KB

I'm wondering how much these tests are actually helpful now, because they basically test now whether new Object()->render == new Object()->render(), this doesn't seem to be that helpful.

I let some of the functions in there, as they are used, so we can remove them once all procedural code can be finally removed.

- // Verify default error message.
- $commands = $this->drupalGetAJAX('ajax-test/render-error');
- $expected = new AlertCommand(t('An error occurred while handling the request: The server received
- $this->assertCommand($commands, $expected->render(), 'ajax_render_error() invokes alert command.'
-

ajax_prepare_response() returned "An error occurred while handling the request: The server received invalid input." when something set an error messages. That's no longer true, as when you have an error you should handle the error all in your custom AlertCommand object.

Status: Needs review » Needs work

The last submitted patch, drupal-1957590-6.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
interdiff.txt500 bytes
drupal-1957590-8.patch20.68 KB

Removed one line too much.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

#8 is awesome! Suppose to get rid of #ajax we need separate issue

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Marked as duplicate #1959574: Remove the deprecated Drupal 7 Ajax API which patch also removes ajax_render()

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

#8: drupal-1957590-8.patch queued for re-testing.

mkadin’s picture

mkadin CreditAttribution: mkadin commented
FileSize
drupal_remove_old_ajax_1957590_11.patch43.2 KB

Here's an extension of the patch from #1959574: Remove the deprecated Drupal 7 Ajax API. Crazy how similar it is to #8 without having seen it!

My work went a bit further to delete a lot of the stuff from ajax.inc that isn't in use anymore, but is otherwise the same (as best I can tell).

Status: Needs review » Needs work

The last submitted patch, drupal_remove_old_ajax_1957590_11.patch, failed testing.

mkadin’s picture

mkadin CreditAttribution: mkadin commented
Status: Needs work » Needs review
FileSize
drupal_remove_old_ajax_1957590_14.patch47.32 KB
interdiff.txt12.62 KB
mkadin’s picture

mkadin CreditAttribution: mkadin commented

That patch incorporates the feedback from #2 and tries to fix the tests.

To trigger AjaxController, I added the custom ajax mime-type to drupalPostAJAX and drupalGetAJAX, but various tests were using drupalGetAJAX to just get / decode json, as opposed to specific ajax commands. So I created WebTestBase::drupalGetJSON() for that purpose, and applied it where needed.

I'm pretty sure this patch will fail, there are 2 fails in the ajax framework tests that I can't for the life of me figure out. They work in the real world, just not in the test environment...so I'm not sure how to debug.

Status: Needs review » Needs work

The last submitted patch, drupal_remove_old_ajax_1957590_14.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

@mkadin

Thanks for getting your work from the other issue in, but I'm asking whether we could get #8 in first.
It's nothing complicated compared to your changes, then we can continue working on it here and maybe on #1938980: Fix Ajax system; the last remnants of the old API must be swept away?

mkadin’s picture

mkadin CreditAttribution: mkadin commented
Status: Needs work » Reviewed & tested by the community

That's fine. In that case, I'll re-open #1959574: Remove the deprecated Drupal 7 Ajax API as postponed on this issue, and add the rest of the stuff there.

I also see #8 as RTBC.

Crell’s picture

Crell CreditAttribution: Crell commented

+1 to committing #8 and continuing in the linked issue(s).

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed #8, following the follow-up :)

Status: Fixed » Closed (fixed)

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