Posted by Crell on November 17, 2012 at 4:33am
15 followers
Issue Summary
My first issue filed against Views VDC! :-)
#1812866: Rebuild the server side AJAX API introduced a new server-side API for Ajax commands that plays nicer with the new Kernel/Request/Response pipeline. The change notice is at http://drupal.org/node/1843212
The Views commands in views/includes/ajax.inc need to be updated for the new API, and possibly refactored in the process. (I defer to the Views team on that front.)
I didn't see an existing issue where this would be relevant, but if I missed one feel free to consolidate.
Comments
#1
I've converted one command, and replaced a couple usages. The rest should follow suit.
#2
+++ 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.
#3
+++ 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.
#4
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
all commands. You could also use the $response object though then we would need a lot of custom setters/getters.
#5
+++ 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.
#6
That looks a bit odd.Just a bit?
#7
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"? ;-)
#8
+++ b/core/modules/views/lib/Drupal/views/Ajax/ViewsAjaxResponse.php@@ -0,0 +1,47 @@
+ protected function ajaxRender($request) {
+ $commands = parent::ajaxRender($request);
+
+ drupal_alter('views_ajax_data', $commands, $this->view);
+ return $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.
#9
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.
Is there some documentation already to at least understand the big picture here?
Okay, so let's assume someone wants to do two things:
What would they do? As written before, there is currently no way from outside a response object to change the values.
#10
If it's really a necessary feature, then we would likely be better off adding a getCommands() method to the AjaxResponse class that simply returns the array of commands by reference. Then code that needs to muck with it can implement the kernel.response listener, check that it's an AjaxResponse, call getCommands(), and molest it as appropriate. That's similar to what the select query builder offers in terms of getters-for-mucking.
#11
Thanks for the idea.
Do we already have an documentation standard how to document available listeners or an example how to use it?
#12
For Drupal-defined ones: #1751116: [policy, then patch] Determine standard for documenting Events and Subscribers/Listeners
For Symfony defined ones: http://symfony.com/doc/current/index.html
#13
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.
#14
"Aware" seems more appropriate in cases where the object is leveraging another object, not when it's just an enhanced data collector. So ViewsAjaxResponse.
That's looking much better there. Not setting RTBC because I've not looked at the Views-specific bits at all. :-) I'll let someone who knows Views properly handle that.
#15
It's now ViewAjaxResponse as it's not about Views but about a view.
Rerolled against HEAD.
#16
The last submitted patch, drupal-1843224-15.patch, failed testing.
#17
#15: drupal-1843224-15.patch queued for re-testing.
#18
+++ 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.
#19
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.
HiliteCommandbuttonS
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.
#20
So I guess we need to add in stuff about altering the responseI 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.#21
I think this looks good, we should probably get a review from an 'Ajax guy' though :)
#22
Crell was happy with it from the Ajax side, and it looks good from the Views side.
#23
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. :)
#24
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.
#25
The last submitted patch, drupal-1843224-24.patch, failed testing.
#26
#24: drupal-1843224-24.patch queued for re-testing.
#27
The last submitted patch, drupal-1843224-24.patch, failed testing.
#28
Rerolled.
#29
Let's wait for #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases
#30
Let's get that better in now and rework later.
#31
I don't really see how the ViewsAjaxResponse is being used here. There's one use of it in the patch, and the code in question doesn't seem to benefit from it.
Otherwise this looks OK to me, but I am not RTBCing because I don't know Views well enough for RTBC-levels of confidence.
#32
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.
#33
Ah, OK. And it's even documented to that effect. :-) I don't see the alter call though. Is that just outside of the patch's code scope, or am I blind?
#34
+++ 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 :)
#35
LOL. OK, I'll stop now. Still not RTBCing because that should be left to someone more familiar with Views.
#36
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.
#37
Looks good to me, I think that follow-up is OK as it is. Committed/pushed to 8.x.
#38
Automatically closed -- issue fixed for 2 weeks with no activity.