Convert Views Ajax commands to new Ajax API

I've converted one command, and replaced a couple usages. The rest should follow suit.

+++ b/core/modules/views/includes/ajax.incundefined
@@ -68,16 +71,15 @@ function views_ajax() {
-    drupal_alter('views_ajax_data', $commands, $view);

We should reconsider the removing as it is pretty helpful: If you just use the generic alter hook (hook_ajax_render_alter) you simple have not the context of the view. There are several contrib modules which uses that hook to inject behavior.

Working on converting the other methods.

+++ b/core/modules/views/includes/ajax.incundefined
@@ -68,16 +71,15 @@ function views_ajax() {
-    drupal_alter('views_ajax_data', $commands, $view);

We should reconsider the removing as it is pretty helpful: If you just use the generic alter hook (hook_ajax_render_alter) you simple have not the context of the view. There are several contrib modules which uses that hook to inject behavior.

Working on converting the other methods.

It seems to be that my actual response got nucked and replaced by the previous one

Here are some points which describes the changes of the patch

• The addTab command was never actually used, as the fast switch of tabs just existed in the old UI
• To be able to support the views_ajax alter hook, we have to implement a custom ajaxResponse object, to be able to access
  all commands. You could also use the $response object though then we would need a lot of custom setters/getters.

+++ b/core/modules/views/lib/Drupal/views/Ajax/SetFormCommand.phpundefined
@@ -0,0 +1,71 @@
+   *   An optional URL of the form..

This should be
(optional) The URL of the form, defaults to NULL.

+++ b/core/modules/views/views.api.phpundefined
@@ -599,11 +599,11 @@ function hook_views_ui_display_top_links_alter(array &$links, ViewExecutable $vi
+ * @param \Drupal\views\ViewExecutable|\ViewExecutable $view

That looks a bit odd.

I agree about the alter, subclassing the response for that case sounds viable.

That looks a bit odd.

Just a bit?

Checked for consistency, seems fine. Covers all occurrences of commands. Fully documented.

+++ b/core/modules/views/views.api.phpundefined
@@ -599,7 +599,7 @@ function hook_views_ui_display_top_links_alter(array &$links, ViewExecutable $vi
- * Alter the commands used on a Views AJAX request.
+ * Alter the response used on a Views AJAX request.

It's the rendered "commands" from the AjaxResponse - the argument received is even $commands. Dunno what naming is better. Possibly even "Alter the response commands"? ;-)

It's the rendered "commands" from the AjaxResponse - the argument received is even $commands. Dunno what naming is better. Possibly even "Alter the response commands"? ;-)

Ups, right i started with converting the documentation when i realized that it's not really useful to alter $response as you don't have access to the commands.

Why do we need an alter here? There's already kernel.response which happens pre-prepare(), if someone really wants to muck with the response object. That's what that event is for. I'm really trying hard to keep alter calls out of the new pieces of the system, as that's where a lot of our non-deterministic spaghetti comes from.

Is there some documentation already to at least understand the big picture here?
Okay, so let's assume someone wants to do two things:

• Adds a new command
• Alter the settings of an existing command

What would they do? As written before, there is currently no way from outside a response object to change the values.

Thanks for the idea.

Do we already have an documentation standard how to document available listeners or an example how to use it?

ViewAjaxResponse or ViewAwareAjaxResponse? Every class using "aware" seems to be connected with the container, but you should know that better :)

Removed the alter hook, added the method and added a todo in the views.api.php file.

It's now ViewAjaxResponse as it's not about Views but about a view.
Rerolled against HEAD.

#15: drupal-1843224-15.patch queued for re-testing.

+++ b/core/lib/Drupal/Core/Ajax/AjaxResponse.phpundefined
@@ -46,6 +46,16 @@ public function addCommand($command, $prepend = FALSE) {
   /**
+   * Returns all the commands.
+   *
+   * @return array
+   *   An array of ajax command arrays.
+   */

Should we doc here that it is returned by reference? And should it be something like 'Returns all stored commands' or 'Returns an array of all commands' instead?

+++ b/core/modules/views/lib/Drupal/views/Ajax/HiliteCommand.phpundefined
@@ -0,0 +1,46 @@
+class HiliteCommand implements CommandInterface {

Should we use HighlightCommand instead and be proper? or we 'HiLite' things in JS, I'm not sure :)

+++ b/core/modules/views/lib/Drupal/views/Ajax/ShowButtonsCommand.phpundefined
@@ -0,0 +1,28 @@
+ * Provides an AJAX command for showing the save and cancel button.

buttonS ?

+++ b/core/modules/views/views_ui/admin.incundefined
@@ -616,29 +621,29 @@ function views_ui_ajax_form($js, $key, ViewUI $view, $display_id = '') {
-  return $js ? array('#type' => 'ajax', '#commands' => $output) : $output;
+  return $response;

So much tidier!

So I guess we need to add in stuff about altering the response? Then I think this is looking pretty ready.

Should we doc here that it is returned by reference? And should it be something like 'Returns all stored commands' or 'Returns an array of all commands' instead?

When i asked in IRC it wasn't clear how to document that something is returned by reference.

HiliteCommand
buttonS

You simply can't argue against a native speaker :)

I guess the way to do it, is to register your event subscriber in the container and then listen to some kind of event.

So I guess we need to add in stuff about altering the response

I think we simply don't have a clue how to document that properly at the moment and it would be somehow the generic way to alter the response.

What about the following?

Returns all the commands by reference.

I think this looks good, we should probably get a review from an 'Ajax guy' though :)

Crell was happy with it from the Ajax side, and it looks good from the Views side.

Hm.

+++ b/core/lib/Drupal/Core/Ajax/AjaxResponse.php
@@ -46,6 +46,16 @@ public function addCommand($command, $prepend = FALSE) {
   /**
+   * Returns all the commands by reference.
+   *
+   * @return array
+   *   An array of ajax command arrays.
+   */
+  public function &getCommands() {
+    return $this->commands;
+  }

I tried to understand the comments #9 and #10... but I'm not sure whether this makes sense?

1) @Crell basically wants each and every custom thing to implement an own command, instead of allowing to alter settings. (He calls it "spaghetti", others would call it "real world needs and use-cases".)

2) The actual, implemented stop-gap workaround allows for much more than what is actually asked for; it allows to retrieve + alter all commands of the response — which essentially equals hook_page_alter(). That's OK-ish (though perhaps also not) for the use-case of a module that wants to alter entire Ajax responses based on various conditions, but that's a completely different thing than the case of 1) creating a command and 2) altering its settings before 3) adding it to the response, no?

3) To achieve that, I'd think that a &getSettings() method on the Ajax command base class would be more appropriate; e.g., like this:

// Create a new Add command.
$add = new AddCommand(...);
// Alter it.
$add->setSetting('foo', 'bar');
// Add it to the response.
$response->addCommand($add);

4) In any case, this method is not used anywhere in this patch, and I think the phpDoc of the method should clarify why this method allows to assign and alter Ajax commands by reference.

If you agree but disagree with hashing out this detail in this issue, then let's remove that getCommands() method from this patch and let's create a follow-up issue to discuss it separately. :)

That's OK-ish (though perhaps also not) for the use-case of a module that wants to alter entire Ajax responses based on various conditions, but that's a completely different thing than the case of 1) creating a command and 2) altering its settings before 3) adding it to the response, no?

So one use-case is to remove the scroll-to-top functionality, so it's not about just altering single ones, but removing them.

Let's remove this command from that patch and discuss that in a follow up.

#24: drupal-1843224-24.patch queued for re-testing.

Rerolled.

Let's wait for #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases

Let's get that better in now and rework later.

People alter the commands of views (there are many contrib modules doing that) so we need some kind of way to let the distinct easily and get the view object as well.

+++ b/core/modules/views/views.api.phpundefined
@@ -594,25 +594,7 @@ function hook_views_ui_display_top_links_alter(array &$links, ViewExecutable $vi
+// @todo Describe how to alter a view ajax response with event listeners.

See here :)

I think that @todo is covered by #1751116: [policy, then patch] Determine standard for documenting Events and Subscribers/Listeners, we're done here!
Amazing work @dawehner.

Looks good to me, I think that follow-up is OK as it is. Committed/pushed to 8.x.