Similar to #1866830: Simplify DisplayPluginBase::validateOptionsForm() we should also try to reduce the amount of code in submitOptionsForm.

CommentFileSizeAuthor
#26 vdc-1888694.patch14.87 KBdawehner
#26 interdiff.txt1.41 KBdawehner
#18 interdiff-1888694-18.txt1.09 KBdamiankloip
#18 1888694-18.patch15.25 KBdamiankloip
#16 interdiff-1888694-16.txt4.99 KBdamiankloip
#16 1888694-16.patch15.27 KBdamiankloip
#13 1888694-13.patch13.7 KBdamiankloip
#6 interdiff.txt1.22 KBdawehner
#6 drupal-1888694-6.patch13.67 KBdawehner
#4 interdiff.txt2.59 KBdawehner
#4 drupal-1888694-4.patch13.89 KBdawehner
#1 drupal-1888694-1.patch11.31 KBdawehner

Comments

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new drupal-1888694-1.patch11.31 KB 
git diff --stat
 .../views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php |  187 +++++++------------------------
 1 file changed, 42 insertions(+), 145 deletions(-)
dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Needs review

Let's see what the testbot tells us.

Status: Needs review » Needs work

The last submitted patch, drupal-1888694-1.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review
StatusFileSize
new drupal-1888694-4.patch13.89 KB
new interdiff.txt2.59 KB

Let's fix the tests.

damiankloip’s picture

damiankloip commented
Status: Needs review » Needs work 
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined
@@ -2179,7 +2143,9 @@ public function submitOptionsForm(&$form, &$form_state) {
+      $this->setOption($section, $form_state['values'][$section]);

@@ -2188,120 +2154,51 @@ public function submitOptionsForm(&$form, &$form_state) {
+      $this->setOption($section, (bool)$form_state['values'][$section]);

The only thing I can find wrong with this patch is the indentation in these two place! Otherwise looks like another awesome cleanup.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review
StatusFileSize
new drupal-1888694-6.patch13.67 KB
new interdiff.txt1.22 KB

Thank you!

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: -VDC

#6: drupal-1888694-6.patch queued for re-testing.

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: +VDC

#6: drupal-1888694-6.patch queued for re-testing.

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: +Novice, +Needs reroll

Status: Needs review » Needs work

The last submitted patch, drupal-1888694-6.patch, failed testing.

david.lukac’s picture

david.lukac commented
Assigned: Unassigned » david.lukac

Assigning to me during DrupalCon Prague sprint.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Assigned: david.lukac » Unassigned
Issue summary: View changes

Unassigning to make this available since it's been a while.

damiankloip’s picture

damiankloip commented
StatusFileSize
new 1888694-13.patch13.7 KB

Reroll.

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 13: 1888694-13.patch, failed testing.

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review
StatusFileSize
new 1888694-16.patch15.27 KB
new interdiff-1888694-16.txt4.99 KB

Needed a few more fixes, also added the getting of options form the plugins, like the pager did before. Since this patch last passed, New assertions in the PagerTest have been added to make sure overridden options are kept. I think this should pass ok now.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Needs work

I really love the diffstat of this patch!

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php
@@ -2246,15 +2246,16 @@ public function submitOptionsForm(&$form, &$form_state) {
+          $plugin = \Drupal::service('plugin.manager.views.' . $plugin_type)->createInstance($form_state['values'][$plugin_type]['type']);

We should probably use Views::pluginManager($pugin_type);

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review
Issue tags: -Novice, -Needs reroll
StatusFileSize
new 1888694-18.patch15.25 KB
new interdiff-1888694-18.txt1.09 KB

Agreed.

Status: Needs review » Needs work

The last submitted patch, 18: 1888694-18.patch, failed testing.

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review

18: 1888694-18.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 18: 1888694-18.patch, failed testing.

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review

18: 1888694-18.patch queued for re-testing.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

I love that diffstat:

 4 files changed, 47 insertions(+), 156 deletions(-)
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Overall this seems like great clean-up, also loving the diffstat.

One question though:

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php
@@ -1627,9 +1627,12 @@ public function buildOptionsForm(&$form, &$form_state) {
-        $form['style'] =  array(
-          '#title' => t('Style'),
-          '#title_display' => 'invisible',
+        $form['style'] = array(
+          '#prefix' => '<div class="clearfix">',
+          '#suffix' => '</div>',
+          '#tree' => TRUE,
+        );
+        $form['style']['type'] =  array(
           '#type' => 'radios',

@@ -1671,9 +1674,12 @@ public function buildOptionsForm(&$form, &$form_state) {
-        $form['row'] =  array(
-          '#title' => t('Row'),
-          '#title_display' => 'invisible',
+        $form['row'] = array(
+          '#prefix' => '<div class="clearfix">',
+          '#suffix' => '</div>',
+          '#tree' => TRUE,
+        );
+        $form['row']['type'] =  array(

Hm. At a glance, didn't we just introduce accessibility bugs here?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs review
dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new interdiff.txt1.41 KB
new vdc-1888694.patch14.87 KB

Good catch, that was probably some stuff which changed during a rerole.

damiankloip’s picture

damiankloip commented
Status: Needs review » Reviewed & tested by the community

Nice work webchick/dawehner. Yeah, that must have crept back in somewhen.

Status: Needs review » Needs work

The last submitted patch, 26: vdc-1888694.patch, failed testing.

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review

26: vdc-1888694.patch queued for re-testing.

damiankloip’s picture

damiankloip commented
Status: Needs review » Reviewed & tested by the community

Thought so.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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