#1913856: Convert Views UI forms to use FormInterface only tackles the non-AJAX forms, because they are much more straightforward.
This attempts to untangle the various AJAX forms that use the Views faux-modal.

The main complication here is the multi-step stacking nature of them. For example, you click to add a new field, you pick two of them, and you go right into configuring the first and the second.

Initially this patch will contain the work done in #1913856: Convert Views UI forms to use FormInterface.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
180.13 KB

Includes the changes from the other issue.

The diffstat in admin.inc is the important part here :)

dawehner’s picture

Status: Needs review » Needs work

Can we get for example #1913718: Aggregation settings link generates AJAX error. in first in order to be safe that we don't break stuff again?

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
180.14 KB

Yes, agreed. I'm going to continue working on this until its green, but that should definitely go in first.

dawehner’s picture

Yeah it's definitive hard to not bike-shed on moves, so I try to give some general comments.

+++ b/core/modules/field/lib/Drupal/field/Plugin/views/field/Field.phpundefined
@@ -410,6 +410,65 @@ public function buildOptionsForm(&$form, &$form_state) {
+  function submitTemporaryForm($form, &$form_state) {

Let's make it public. In general this field should certainly not be part of the Field class ... maybe better the Handlerbase, but it's adding code on runtime, which is actually just needed in the Admin UI. Do you think it's worth to add a small helper class for this?

+++ b/core/modules/field/lib/Drupal/field/Plugin/views/field/Field.phpundefined
@@ -410,6 +410,65 @@ public function buildOptionsForm(&$form, &$form_state) {
+    // Add the incoming options to existing options because items using
+    // the extra form may not have everything in the form here.

We should fix this comments-80-chars later.

+++ b/core/modules/field/lib/Drupal/field/Plugin/views/field/Field.phpundefined
@@ -410,6 +410,65 @@ public function buildOptionsForm(&$form, &$form_state) {
+    views_ui_cache_set($form_state['view']);

opened an unrelated follow-up #1919700: [Change notice[ Replace views_ui_cache_set with a method on the ViewUI object

public

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined
@@ -2290,7 +2322,7 @@ public function submitOptionsForm(&$form, &$form_state) {
+              $form_state['view']->addFormToStack('display', $this->display['id'], 'pager_options');

See comment below.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/filter/FilterPluginBase.phpundefined
@@ -407,11 +407,35 @@ function show_build_group_button(&$form, &$form_state) {
+  function buildGroupForm($form, &$form_state) {

... public again.

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/Form/ConfigItem.phpundefined
@@ -0,0 +1,237 @@
+  public function ajaxForm(ViewStorageInterface $view, $display_id, $js, $type = NULL, $id = NULL) {
+    $this->setType($type);
+    $this->setID($id);
+    return parent::ajaxForm($view, $display_id, $js);

Stuff like that really look nice!

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/Form/ViewsFormBase.phpundefined
@@ -0,0 +1,209 @@
+  public function ajaxForm(ViewStorageInterface $view, $display_id, $js) {

The docs doesn't seem to match anymore :)

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/Form/ViewsFormBase.phpundefined
@@ -0,0 +1,209 @@
+      // @todo Consider replacing this with an array property mapping them, or
+      //   forms as plugins.

Forms as plugins seems to be 100% over-enginering, but yeah I agree that using pre_replace here is wrong.

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewUI.phpundefined
@@ -361,132 +368,10 @@ public function getOverrideValues($form, $form_state) {
-  public function addFormToStack($key, $display_id, $args, $top = FALSE, $rebuild_keys = FALSE) {
+  public function addFormToStack($key, $display_id, $type, $id = NULL, $top = FALSE, $rebuild_keys = FALSE) {

Do we really have to rewrite all of that here? This potentially removes the flexibility of the function, without any gain, Oh i see you have type and ID now on the form base class.

+++ b/core/modules/views/views_ui/views_ui.routing.ymlundefined
@@ -67,7 +67,7 @@ views_ui.delete:
-    _controller: 'views_ui.controller:deleteForm'
+    _controller: 'Drupal\views_ui\Form\ViewsUIDelete::getForm'

It feels just wrong to have both of them, but well.

tim.plunkett’s picture

FileSize
13.9 KB
181.56 KB

I fixed the public on the methods, and the outdated docs on ajaxForm(), nice catch.

We talked about the changes to addFormToStack(), so I'm leaving that in.

The mapping of form_keys to classes is the main problem left to solve here.

The Drupal\views_ui\Form\ViewsUIDelete change I'm leaving up to #1915774: Decide whether core route controllers should generally/always be DIC services or not, but I didn't see the reason to add a service when it didn't need anything injected.

Fixed some of the tests failures.

Also, removed the $third paramater from getStandardButtons(), since it was only used in once place and was really an unneeded abstraction. Also, it only worked for functions.

tim.plunkett’s picture

FileSize
1.7 KB
183.26 KB

My last bugfix exposed two test inconsistencies, fixed here.

tim.plunkett’s picture

FileSize
193.02 KB

Rerolled on top of the latest patch in the blocking issue.

dawehner’s picture

Issue tags: -VDC

#10: vdc-1919142-10.patch queued for re-testing.

tim.plunkett’s picture

Issue tags: +VDC
FileSize
193.03 KB
1.38 KB

Whoops, missed the validate->validateForm switch.

tim.plunkett’s picture

FileSize
147.99 KB
tim.plunkett’s picture

FileSize
2.49 KB
148.34 KB

Okay, finally removed that preg_replace_callback. Leaving assigned in case I have to fix tests, but otherwise I'd like to be done with this (and the Views UI!).

tim.plunkett’s picture

Issue tags: +FormInterface

Tagging.

tim.plunkett’s picture

FileSize
147.95 KB

This included the same fix as in #1913718: Aggregation settings link generates AJAX error., so it needed a reroll.

damiankloip’s picture

Status: Needs review » Needs work

Mostly some nitpicks, overall looks great!

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/HandlerBase.phpundefined
@@ -937,4 +937,86 @@ public static function breakPhraseString($str, &$handler = NULL) {
+    // flip
+    $item['exposed'] = empty($item['exposed']);

Having this logic seems weird?

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/filter/FilterPluginBase.phpundefined
@@ -407,11 +407,35 @@ function show_build_group_button(&$form, &$form_state) {
+    // flip. If the filter was a group, set back to a standard filter.
+    $item['is_grouped'] = empty($item['is_grouped']);

Same here, to nitpick though, capital Flip. Or are these slightly out of scope for this issue?

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/filter/FilterPluginBase.phpundefined
@@ -1058,6 +1082,23 @@ function build_group_form(&$form, &$form_state) {
+   * Add a new group to the exposed filter groups.

Adds

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/Form/Ajax/ConfigItem.phpundefined
@@ -0,0 +1,259 @@
+   * Overrides ViewsFormBase::validateForm().
...
+   * Overrides ViewsFormBase::submitForm().

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/Form/Ajax/ConfigItemExtra.phpundefined
@@ -0,0 +1,112 @@
+   * Overrides ViewsFormBase::validateForm().
...
+   * Overrides ViewsFormBase::submitForm().

Namespaces

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/Form/Ajax/ConfigItemExtra.phpundefined
@@ -0,0 +1,112 @@
+ * @todo.

Need to fill this in.

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/Form/Ajax/ConfigItemGroup.phpundefined
@@ -0,0 +1,106 @@
+ * @todo.

.

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/Form/Ajax/ConfigItemGroup.phpundefined
@@ -0,0 +1,106 @@
+   * Overrides ViewsFormBase::submitForm().

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/Form/Ajax/Display.phpundefined
@@ -0,0 +1,110 @@
+   * Overrides ViewsFormBase::validateForm().
...
+   * Overrides ViewsFormBase::submitForm().

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/Form/Ajax/EditDetails.phpundefined
@@ -0,0 +1,85 @@
+ * @todo.
...
+   * Overrides ViewsFormBase::submitForm().

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/Form/Ajax/Rearrange.phpundefined
@@ -0,0 +1,158 @@
+ * @todo.
...
+   * Overrides ViewsFormBase::submitForm().

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/Form/Ajax/RearrangeFilter.phpundefined
@@ -0,0 +1,315 @@
+ * @todo.
...
+   * Overrides ViewsFormBase::submitForm().

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/Form/Ajax/ReorderDisplays.phpundefined
@@ -0,0 +1,137 @@
+ * @todo.
...
+   * Overrides ViewsFormBase::submitForm().

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/Form/Ajax/ViewsFormBase.phpundefined
@@ -0,0 +1,174 @@
+    module_load_include('inc', 'views_ui', 'admin');

I guess we still need this in here?

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewUI.phpundefined
@@ -121,52 +121,22 @@ class ViewUI implements ViewStorageInterface {
   public static $forms = array(
+    'add-item' => '\Drupal\views_ui\Form\Ajax\AddItem',
+    'analyze' => '\Drupal\views_ui\Form\Ajax\Analyze',
+    'config-item' => '\Drupal\views_ui\Form\Ajax\ConfigItem',
+    'config-item-extra' => '\Drupal\views_ui\Form\Ajax\ConfigItemExtra',
+    'config-item-group' => '\Drupal\views_ui\Form\Ajax\ConfigItemGroup',
+    'display' => '\Drupal\views_ui\Form\Ajax\Display',
+    'edit-details' => '\Drupal\views_ui\Form\Ajax\EditDetails',
+    'rearrange' => '\Drupal\views_ui\Form\Ajax\Rearrange',
+    'rearrange-filter' => '\Drupal\views_ui\Form\Ajax\RearrangeFilter',

Nice!

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
20.04 KB
151.75 KB

There's some new standard that says you don't need the full namespace, even in docs, when its the same namespace. I don't agree, but that's what it is.

I'm not fixing the docs/code that was 100% moved, at least not right away.

I did resolve all of the @todos.

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

Yeah, that seems inconsistent to me. Where is the issue for that? It seems as crazy as removing ALL uses of md5, just for the sake of it ;) Oh well.

Anyway, those changes look good. This is ready. Nice patch.

dawehner’s picture

Yeah, that seems inconsistent to me. Where is the issue for that? It seems as crazy as removing ALL uses of md5, just for the sake of it ;) Oh well.

I see the point that this makes it indeed harder for non-IDE folks to follow what's going on. As reading is more important then writing code, I personally on your site.

tim.plunkett’s picture

FileSize
152.57 KB

I spoke to klausi, a proponent of that standard, and it turns out it was removed because it was an inconsistency. So yay for that.
Just changed those parts of the docblock, leaving RTBC.

tim.plunkett’s picture

FileSize
13.18 KB

Er, interdiff to prove it

damiankloip’s picture

Super. IMO that was a bad idea anyway :)

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Ok, this is the last of the major refactorings to bring Views UI into line with D8, so hooray for that. I do have a bunch of things I found when going through, which are detailed below. I think these can be handled in follow-ups though, since Tim was pretty adamant that the scope of this issue be just purely porting what's there, not cleaning it up as well.

My main confusion was around getKey() vs. getFormID() (and there's also $view->id()). I get (now) that one is in for building a URL to the form (the PHPDoc needs to be amended slightly for that) and the other is used for the form itself, but from a module developer perspective it's still really weird that I need to invent two unique strongs that both refer to the same form. It'd be much better if at least one of these could be auto-derived from the class name in order to eliminate boilerplate stuff that's thrust onto humans to figure out. Tim said that was tried with getKey(), but resulted in some pretty gnarly code (see http://drupal.org/files/interdiff_3349.txt).

I also said that it may make sense to move this to the "upstream" form interfaces, because depending on how #1886616: Multistep Form Wizard comes along, it'd be nice if this stacking system could use the same mechanisms rather than having two multistep form creation things in core. However, that's not ready, and this is, so makes sense to move forward on it.

Most of the rest of this is pretty minor, other than please, please, PLEASE open a follow-up to expand on the comments that say things like "// flip." to explain why they are inverting the value of various variables in a most confusing manner. :P~

Committed and pushed to 8.x. Thanks. Other feedback is below, marking needs work for that.

+++ b/core/modules/field/lib/Drupal/field/Plugin/views/field/Field.phpundefined
@@ -365,7 +365,7 @@ public function buildOptionsForm(&$form, &$form_state) {
+      '#submit' => array(array($this, 'submitTemporaryForm')),

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined
@@ -1837,7 +1837,7 @@ public function buildOptionsForm(&$form, &$form_state) {
+          '#submit' => array(array($this, 'changeThemeForm')),

@@ -1851,7 +1851,7 @@ public function buildOptionsForm(&$form, &$form_state) {
+          '#submit' => array(array($this, 'rescanThemes')),

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/filter/FilterPluginBase.phpundefined
@@ -398,7 +398,7 @@ function show_build_group_button(&$form, &$form_state) {
+        '#submit' => array(array($this, 'buildGroupForm')),

@@ -407,11 +407,35 @@ function show_build_group_button(&$form, &$form_state) {
+        '#submit' => array(array($this, 'buildGroupForm')),

@@ -443,7 +467,7 @@ public function showExposeButton(&$form, &$form_state) {
+        '#submit' => array(array($this, 'displayExposedForm')),

@@ -455,7 +479,7 @@ public function showExposeButton(&$form, &$form_state) {
+        '#submit' => array(array($this, 'displayExposedForm')),

@@ -1038,7 +1062,7 @@ function build_group_form(&$form, &$form_state) {
+      '#submit' => array(array($this, 'addGroupForm')),

It's odd that most of these end in Form but not rescanThemes. We should make this consistent, no?

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/HandlerBase.phpundefined
@@ -937,4 +937,86 @@ public static function breakPhraseString($str, &$handler = NULL) {
+   * Displays the Expose form.
...
+  public function displayExposedForm($form, &$form_state) {

(Minor) is it "Exposed" or "Expose"? The name/comment should match.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/HandlerBase.phpundefined
@@ -937,4 +937,86 @@ public static function breakPhraseString($str, &$handler = NULL) {
+    // flip
+    $item['exposed'] = empty($item['exposed']);

I realize this is just copy/paste code, but this kind of code/comment combo is not ok. :P Let's get a follow-up to clean this up.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/HandlerBase.phpundefined
@@ -937,4 +937,86 @@ public static function breakPhraseString($str, &$handler = NULL) {
+  /**
+   * A submit handler that is used for storing temporary items when using
+   * multi-step changes, such as ajax requests.
+   */

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined
@@ -2021,6 +2021,38 @@ public function buildOptionsForm(&$form, &$form_state) {
   /**
+   * Submit hook to clear Drupal's theme registry (thereby triggering
+   * a templates rescan).
+   */

(minor) Let's knock this down to 80 chars (+another para if necessary).

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/HandlerBase.phpundefined
@@ -937,4 +937,86 @@ public static function breakPhraseString($str, &$handler = NULL) {
+    // @todo Decide if \Drupal\views_ui\Form\Ajax\ViewsFormBase::getForm() is
+    //   perhaps the better place to fix the issue.
+    // \Drupal\views_ui\Form\Ajax\ViewsFormBase::getForm() drops the current
+    // form from the stack, even if it's an #ajax. So add the item back to the top
+    // of the stack.
+    $form_state['view']->addFormToStack($form_state['form_key'], $form_state['display_id'], $type, $item['id'], TRUE);

That seems worth a follow-up as well.

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/Form/Ajax/AddItem.phpundefined
@@ -0,0 +1,187 @@
+  public function getFormKey() {
+    return 'add-item';
+  }
...
+  public function getFormID() {
+    return 'views_ui_add_item_form';
+  }

It's confusing to me that we have two functions that basically say "give me a unique name of this thing." can we not just re-use getFormID()?

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/Form/Ajax/Analyze.phpundefined
@@ -0,0 +1,63 @@
+  /**
+   * Overrides \Drupal\views_ui\Form\Ajax\ViewsFormBase::submitForm().
+   */
+  public function submitForm(array &$form, array &$form_state) {
+    $form_state['redirect'] = 'admin/structure/views/view/' . $form_state['view']->id() . '/edit';
+  }

Yet another ID we use in the URL? :)

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/Form/Ajax/ViewsFormInterface.phpundefined
@@ -0,0 +1,60 @@
+  /**
+   * Returns the key that represents this form.
+   *
+   * @return string
+   *   The form key used in the URL, e.g., the string 'add-item' in
+   *   'admin/structure/views/%/add-item/%/%/%'.
+   */
+  public function getFormKey();

While the @return does a pretty good job of explaining this, the actual PHPDoc that will show up in IDEs is pretty terrible. Could we expand it to at least have the word "URL" in it somewhere? An example might be good too, to explain that this should be all lower-case with hyphens separating the words.

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewUI.phpundefined
@@ -121,52 +121,22 @@ class ViewUI implements ViewStorageInterface {
+  /**
+   * Contains an array of form keys and their respective classes.
+   *
+   * @var array
+   */
   public static $forms = array(
...
-    'analyze' => array(
-      'form_id' => 'views_ui_analyze_view_form',
-      'args' => array(),
-    ),
+    'add-item' => '\Drupal\views_ui\Form\Ajax\AddItem',
+    'analyze' => '\Drupal\views_ui\Form\Ajax\Analyze',
+    'config-item' => '\Drupal\views_ui\Form\Ajax\ConfigItem',

This cleaned up a lot of boiler-plate code, which is nice. However, it seems like we could clean up even more if we just auto-generated the form key from the class name. Each of these is exactly the same in that regard.

dawehner’s picture

dawehner’s picture

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
andypost’s picture

Issue summary: View changes

This issue blocks #1791352: When changes are made for all displays, mark all displays as changed which is a long standing bug in D7

dawehner’s picture

This issue blocks #1791352: When changes are made for all displays, mark all displays as changed which is a long standing bug in D7

I don't understand why this is blocking it. As webchick wrote in her comment:

Committed and pushed to 8.x. Thanks. Other feedback is below, marking needs work for that.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
1.28 KB

This issue was already committed. Addressing some minor followups here and opening a new issue for a more complicated one.

  1. It's odd that most of these end in Form but not rescanThemes. We should make this consistent, no? Code Removed in favor of Twig.
  2. I realize this is just copy/paste code, but this kind of code/comment combo is not ok.Patched
  3. (Minor) is it "Exposed" or "Expose"? The name/comment should match. Patched
  4. That seems worth a follow-up as well. #2676618: Follow up for a @todo: Decide if ViewsFormBase::getForm() is the better place to fix the issue.
  5. (minor) Let's knock this down to 80 chars (+another para if necessary).Code Removed in favor of Twig.
  6. It's confusing to me that we have two functions that basically say "give me a unique name of this thing." can we not just re-use getFormID()? I don't get this either
  7. Yet another ID we use in the URL? :) Isn't this the view id as opposed to the AJAX form ID?
  8. Returns the key that represents this form. Patched
  9. Contains an array of form keys and their respective classes. We now have a couple that don't match the pattern.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

Method \Drupal\views_ui\Form\Ajax\ViewsFormBase::getForm() contains following lines

      // Build the new form state for the next form in the stack.
      $reflection = new \ReflectionClass($view::$forms[$top[1]]);

But $view::$forms - variable $forms is unknown

lokapujya’s picture

@andypost Maybe open a followup for that?

Can #33 be committed or should I move to a followup?

Also, I opened: https://www.drupal.org/node/2676618

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Changes make sense!

PS no idea how to name the follow-up

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

So in answer to

Can #33 be committed or should I move to a followup?

In the last 3 years our followup process has become quite different. Please create a new issue with the patch in #33 - the issue title and summary bear no relation to what the issue is doing.

alexpott’s picture

Status: Fixed » Closed (fixed)

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