At the moment there are 3 views_ui_pre_render_* functions that live in views_ui/includes/admin.inc These are then added as pre_render callbacks in views plugins. This doesn't make sense, as everything else to do with buildForm etc... lives with the classes. These should be moved in the appropriate views plugin classes and called from there as methods.

This is reliant on #1993312: Change pre_render, post_render, and after_build callbacks to accept callables, so waiting on that.

CommentFileSizeAuthor
#20 interdiff-1993384-20.txt5.93 KBdamiankloip
#20 1993384-20.patch11.59 KBdamiankloip
#16 1993384-16.patch11.5 KBgrisendo
#8 1993384-8.patch10.97 KBdamiankloip
#8 interdiff-1993384-8.txt3.7 KBdamiankloip
vdc.views-ui-pre-render.patch9.97 KBdamiankloip
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Issue tags: +VDC

.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Postponed » Needs review
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented 
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/PluginBase.phpundefined
@@ -331,4 +331,52 @@ public function globalTokenForm(&$form, &$form_state) {
+   * Pre render: Move form elements into details for presentation purposes.
+   *
+   * Many views forms use #tree = TRUE to keep their values in a hierarchy for
+   * easier storage. Moving the form elements into fieldsets during form building
+   * would break up that hierarchy. Therefore, we wait until the pre_render stage,
+   * where any changes we make affect presentation only and aren't reflected in
+   * $form_state['values'].

I know these were copy paste, but we can clean them up a bit. If anything, we have to rewrap them to fit

Also, do we have any desire to remove the need for these? They just seem like FAPI abuse.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/PluginBase.phpundefined
@@ -331,4 +331,52 @@ public function globalTokenForm(&$form, &$form_state) {
+   * Pre render: Move form elements into details for presentation purposes.

Seems a little odd as coding standard.

Status: Needs review » Needs work
Issue tags: -VDC

The last submitted patch, vdc.views-ui-pre-render.patch, failed testing.

dawehner’s picture

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

vdc.views-ui-pre-render.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +VDC

The last submitted patch, vdc.views-ui-pre-render.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
interdiff-1993384-8.txt3.7 KB
1993384-8.patch10.97 KB

I think we need to keep these, as they are specifically pre render callback so they can alter the form but we don't alter the $form_state['values'] structure.

Status: Needs review » Needs work
Issue tags: -VDC

The last submitted patch, 1993384-8.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
Issue tags: +VDC

#8: 1993384-8.patch queued for re-testing.

dawehner’s picture

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

So let's move them further up.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Yeah, let's do that.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
Issue tags: -VDC

#8: 1993384-8.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +VDC

The last submitted patch, 1993384-8.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue tags: +Needs reroll

.

grisendo’s picture

grisendo CreditAttribution: grisendo commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1993384-16.patch11.5 KB

Re-rolled

Status: Needs review » Needs work

The last submitted patch, 16: 1993384-16.patch, failed testing.

grisendo’s picture

grisendo CreditAttribution: grisendo commented
Status: Needs work » Needs review

16: 1993384-16.patch queued for re-testing.

grisendo’s picture

grisendo CreditAttribution: grisendo commented

Green now! Broken head before?

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
1993384-20.patch11.59 KB
interdiff-1993384-20.txt5.93 KB

Thanks grisendo! Looks good. One more thing; I think we should not use $this in the pre render calls, everywhere else we are just doing get_class($this) nowadays.

So maybe just like this?

dawehner’s picture

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

Ship it.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented

20: 1993384-20.patch queued for re-testing.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented

20: 1993384-20.patch queued for re-testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Assigned: damiankloip » Unassigned

I don't need to be assigned to this now.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 004fa12 and pushed to 8.x. Thanks!

[Edit] Removed a documentation fix I said I did when I did not.

Status: Fixed » Closed (fixed)

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