Convert views_ajax() to a Controller

Since this is an AJAX callback rather than a full page callback, we should wait on #1944472: Add generic content handler for returning dialogs.

#1944472: Add generic content handler for returning dialogs is in.

I started already but sadly there are still problems with exposed forms.

There we go.

#5: drupal-1979036-5.patch queued for re-testing.

awesome!

+++ b/core/modules/views/lib/Drupal/views/Controller/ViewAjaxController.phpundefined
@@ -0,0 +1,136 @@
+          _current_path($path);

lets use the $request here
$request->attributes->get('system_path');

_current_path will hopefully die before release

Cool.

I guess it's not ideal to use request::get() in a controller like this? I guess it makes sense in this case though?

I think it probably does.

Given that this involves AJAX, I'd like to see us do thorough manual testing of this patch. Could someone please do that and confirm that things still work? Thanks!

Also, re: #4, it sounds like there was a bug, but the patch still passed? Is there something we could add an automated test for?

Manually testing today at Acquia sprint.

Not sure about #13 / #4 but the patch seems to work for me.

http://screencast.com/t/exrj6JdaTv3X

The problem mentioned on #1979036-4: Convert views_ajax() to a Controller was fixed by #1979036-5: Convert views_ajax() to a Controller

+++ b/core/modules/views/views.moduleundefined
@@ -296,13 +296,9 @@ function views_menu() {
   $items['views/ajax'] = array(
     'title' => 'Views',
-    'page callback' => 'views_ajax',
     'theme callback' => 'ajax_base_page_theme',
-    'delivery callback' => 'ajax_deliver',
-    'access callback' => TRUE,
-    'description' => 'Ajax callback for view loading.',
+    'route_name' => 'views_ajax',
     'type' => MENU_CALLBACK,
-    'file' => 'includes/ajax.inc',
   );

Shouldn't we be removing this? I guess we've left this in for the theme callback... have you tried removing it?

So alternative to "theme callback" it would be possible to temporally port it to use hook_custom_theme for now.

#9: drupal-1979036-9.patch queued for re-testing.

See #1954892: Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system as related issue.

Fixed some minor stuff.

totally forgot this one

+++ b/core/modules/views/lib/Drupal/views/Controller/ViewAjaxController.phpundefined
@@ -0,0 +1,146 @@
+   *   The factory to load a view executable.

The factory for loading a view executable... or: The factory to load a view executable with.

+++ b/core/modules/views/lib/Drupal/views/Controller/ViewAjaxController.phpundefined
@@ -0,0 +1,146 @@
+   * Loads and render a view via AJAX.

Loads and renderS a view via Ajax.

+++ b/core/modules/views/lib/Drupal/views/Controller/ViewAjaxController.phpundefined
@@ -0,0 +1,146 @@
+   *  The current request object.

Missing whitespace before the "The"

+++ b/core/modules/views/lib/Drupal/views/Controller/ViewAjaxController.phpundefined
@@ -0,0 +1,146 @@
+      // Remove all of this stuff from the query of the request so it doesn't end

Exceeds 80 characters.

+++ b/core/modules/views/lib/Drupal/views/Controller/ViewAjaxController.phpundefined
@@ -0,0 +1,146 @@
+        if ($request->query->has($key)) {
+          $request->query->remove($key);
+        }
+        if ($request->request->has($key)) {
+          $request->request->remove($key);
+        }

Those has() checks are redundant. Simply call ->remove() without them.

+++ b/core/modules/views/lib/Drupal/views/Controller/ViewAjaxController.phpundefined
@@ -0,0 +1,146 @@
+        $query = Url::buildQuery($request->query->all());

You already filled $query_all before, you can re-use that here :)

Thanks for the good review.

You already filled $query_all before, you can re-use that here :)

No we can't, because the values have been changed in the meantime.

RTBC if green

+++ b/core/modules/views/views.module
@@ -297,13 +297,9 @@ function views_menu() {
   $items['views/ajax'] = array(
     'title' => 'Views',
-    'page callback' => 'views_ajax',
     'theme callback' => 'ajax_base_page_theme',
-    'delivery callback' => 'ajax_deliver',
-    'access callback' => TRUE,
-    'description' => 'Ajax callback for view loading.',
+    'route_name' => 'views_ajax',
     'type' => MENU_CALLBACK,
-    'file' => 'includes/ajax.inc',
   );

This is a MENU_CALLBACK, which means it no longer needs to exist, I believe. It can be removed entirely.

Crell, you followed in the steps of alexpott, but there is still 'theme callback' which is needed, see #1979036-19: Convert views_ajax() to a Controller and #1979036-20: Convert views_ajax() to a Controller

D'Oh. At least I have good role models. :-)

#26: vdc-1979036-26.patch queued for re-testing.

Committed 338705a and pushed to 8.x. Thanks!

Really? We need a change notice for an ajax path internal to views? We haven't created change notices for all of our route conversions.

I totally agree with damian. We don't convert when page callbacks gets converted to controllers.