Refactor Page based display plugin classes

Great improvement! I know that many of the code is just moving around, so i don't nitpick on that.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/CallbackPluginBase.phpundefined
@@ -0,0 +1,292 @@
+abstract class CallbackPluginBase extends DisplayPluginBase {

I'm not sure whether the classname helps to understand the purpose behind it. The name doesn't tell you that hook_menu is used. What about ControllerPluginBase so Controller in terms of symfony router controller.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/CallbackPluginBase.phpundefined
@@ -0,0 +1,292 @@
+        if (strpos($form_state['values']['path'], '$arg') !== FALSE) {
+          form_error($form['path'], t('"$arg" is no longer supported. Use % instead.'));
+        }

Just in general we should be able to drop that now. $arg was used in d5

OH sure that is a good name, at least for now.

Here's some feedback. Sorry if I'm asking dumb questions, this is my first time in a long time really looking deep into Views code.

A lot of these are nitpicks, but one key piece of feedback is:

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/Feed.phpundefined
@@ -28,7 +26,7 @@
-class Feed extends Page {
+class Feed extends PathPluginBase {

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/Page.phpundefined
@@ -27,7 +27,7 @@
-class Page extends DisplayPluginBase {
+class Page extends PathPluginBase {

I will say I find the old version of this a lot more understandable. :(

Can Path not be a "decorator" type pattern, rather than a brand new thing from which Page-based displays to derive?

Basically, I would love more information on why this is a good idea, or what specific problems we're trying to solve here that would make the "learnability" trade-off worth it.

Here's the rest.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined
@@ -333,9 +333,11 @@ public function useMoreText() {
+    // If the display doesn't use attachments, or it's the current display.
+    if (!$this->usesAttachments() || ($this->definition['id'] == $this->view->current_display)) {

That indeed describes what the code down below says, but it doesn't tell me why. Why do we disable attachments on the current display?

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/Feed.phpundefined
@@ -57,47 +55,60 @@ public function init(ViewExecutable $view, &$display, $options = NULL) {
+    parent::execute();

@@ -124,12 +138,12 @@ protected function defineOptions() {
-    // It is very important to call the parent function here:
     parent::optionsSummary($categories, $options);

@@ -213,11 +224,9 @@ public function buildOptionsForm(&$form, &$form_state) {
-    // It is very important to call the parent function here:
     parent::submitOptionsForm($form, $form_state);

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/Page.phpundefined
@@ -561,22 +363,13 @@ public function buildOptionsForm(&$form, &$form_state) {
-    // It is very important to call the parent function here:
     parent::validateOptionsForm($form, $form_state);

@@ -598,13 +391,13 @@ public function validateOptionsForm(&$form, &$form_state) {
-    // It is very important to call the parent function here:
     parent::submitOptionsForm($form, $form_state);

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.phpundefined
@@ -0,0 +1,288 @@
+    parent::optionsSummary($categories, $options);
...
+    // It is very important to call the parent function here:
+    parent::buildOptionsForm($form, $form_state);
...
+    // It is very important to call the parent function here:
+    parent::validateOptionsForm($form, $form_state);
...
+    // It is very important to call the parent function here:
+    parent::submitOptionsForm($form, $form_state);

There are some hunks in this patch where you remove this comment, and then others where you add it, and others where you add a call to the parent but don't comment it. Why? Seems like we should be consistent.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/Feed.phpundefined
@@ -124,12 +138,12 @@ protected function defineOptions() {
-    // Since we're childing off the 'page' type, we'll still *call* our
-    // category 'page' but let's override it so it says feed settings.

Isn't that still relevant?

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.phpundefined
@@ -0,0 +1,288 @@
+    // Replace % with the link to our standard views argument loader
+    // views_arg_load -- which lives in views.module
...
+    // Get access callback might return an array of the callback + the dynamic arguments.
...
+        // default views page entry
...
+        // Default access check (per display)
...
+        // Identify URL embedded arguments and correlate them to a handler

These are all just copy/paste comments, so no need to fix them here, but let's get a follow-up cos there are a number of standards violations here ("Capital" on start, ending with period, 80 char wrapping, etc.)

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.phpundefined
@@ -0,0 +1,288 @@
+      // Add context for contextual links.
+      // @see menu_contextual_links()
+      if (!empty($menu['context'])) {
+        $items[$path]['context'] = MENU_CONTEXT_INLINE;
+      }

Doesn't that go in contextual.module, per #1817698: Move contextual links field handler out of views.views.inc and put it into contextual_links module?

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.phpundefined
@@ -0,0 +1,288 @@
+      'value' => views_ui_truncate($path, 24),

views_ui_truncate? Ew. :) Seems like we need to abstract that into a general helper function (looks very similar to _filter_url_trim). Separate issue, obviously.

+++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/DisplayFeedTest.phpundefined
@@ -58,7 +58,7 @@ public function testFeedUI() {
-    $this->assertEqual($options, array('default', 'feed', 'page'), 'Make sure all displays appears as expected.');
+    $this->assertEqual($options, array('default', 'page'), 'Make sure all displays appears as expected.');

Didn't we just remove some test coverage?

I will say I find the old version of this a lot more understandable. :(

Can Path not be a "decorator" type pattern, rather than a brand new thing from which Page-based displays to derive?

Basically, I would love more information on why this is a good idea, or what specific problems we're trying to solve here that would make the "learnability" trade-off worth it.

Maybe we have to find a better name for the hook_menu base-class, so it would be easier to get? A longer explanation for the route follows.

So we have a really generic class (DisplayPluginBase) which allows a view to be displayed anywhere (block, page, panels etc.).
A common use-case is to have a view be displayed on a path, so there should be another base-class which provides the fundamental features
required for a hook_menu entry + callback. The previous code instead extended Feed from Page (an actual page with blocks etc.) and removed the features which aren't required for a feed, so we basically try to clean things up. For me the decorator pattern seems to be a way to extend classes horizontally, which is not required here, and so would be harder to understand.

That indeed describes what the code down below says, but it doesn't tell me why. Why do we disable attachments on the current display?

Does this help better to understand it?

// To be able to accept attachments this display have to be able to use
// attachments but at the same time, you cannot attach a display to itself.

There are some hunks in this patch where you remove this comment, and then others where you add it, and others where you add a call to the parent but don't comment it. Why? Seems like we should be consistent.

This happens if you just move code around :) I removed the all, because this really doesn't improve it.

Isn't that still relevant?

You are right, it should document it, but use path type.

These are all just copy/paste comments, so no need to fix them here, but let's get a follow-up cos there are a number of standards violations here ("Capital" on start, ending with period, 80 char wrapping, etc.)

Sounds like a good follow-up issue: #1825230: Fix documentation standards in PathPluginBase/Feed/Page

Doesn't that go in contextual.module, per #1817698: Move contextual links field handler out of views.views.inc and put it into contextual_links module?

Good question. At least the constant and menu_contextual_links is part of menu.inc. I'm not sure but it could be hard to understand, if contextual module is doing some additional altering.

Didn't we just remove some test coverage?

The test was actually wrong, you should not be able to attach a display to itself. This caused the error in #1821654: Refactor Page based display plugin classes and got fixed by the
second hunk you reviewed above.

#23: 1821654-23.patch queued for re-testing.

We need the refactoring in, and we can cleanup later.

#23: 1821654-23.patch queued for re-testing.

Other than a couple nitpicks, I think this is RTBC.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/Feed.phpundefined
@@ -57,47 +55,60 @@ public function init(ViewExecutable $view, &$display, $options = NULL) {
+   * Overrides Drupal\views\Plugin\views\display\DisplayPluginBase::getStyleType().

Here and everywhere else, should be \Drupal

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.phpundefined
@@ -0,0 +1,285 @@
+ * Definition of Drupal\views\Plugin\views\display\PathPluginBase.

Switch to "Contains"

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.phpundefined
@@ -0,0 +1,285 @@
+    switch ($form_state['section']) {
+      case 'path':
...
+    switch ($form_state['section']) {
+      case 'path':
...
+    switch ($form_state['section']) {
+      case 'path':

It seems ridiculous to have a switch with only one case when the code has lasted like this for so long.

Thanks!

Committed and pushed to 8.x. Thanks!