Currently there is
ViewExecutable::renderField() (never used)
StylePluginBase::render_fields() (an internal set-up method)
StylePluginBase::get_field() (the actual API method)

In design, StylePluginBase::render_fields() is used to actual perform the rendering, and stores the results, while StylePluginBase::get_field() is used to retrieve those results.

In reality, several places call StylePluginBase::render_fields() directly and circumvent the implicit caching done by StylePluginBase::get_field().

CommentFileSizeAuthor
#12 drupal-1861838-12.patch7.75 KBdawehner
#7 vdc-1861838-7.patch7.7 KBtim.plunkett
#7 interdiff.txt4.22 KBtim.plunkett
#1 vdc-1861838-1.patch3.82 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Active » Needs review
FileSize
vdc-1861838-1.patch3.82 KB

See attached.

Lars Toomre’s picture

Lars Toomre CreditAttribution: Lars Toomre commented

Since StylePluginBase::render_fields() is being turned into an internal protected method, does it make sense to change the method name to conform with camelCase naming convention too?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Keep issues as simple as possible is the key to get stuff in.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/style/StylePluginBase.phpundefined
@@ -622,19 +622,20 @@ function render_fields($result) {
-    return $this->rendered_fields;

This feels wrong, as it is a helpful thing when you just want to get that information of check for non-empty result.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +Needs change record

Please move this issue to the Views 8.x-3.x branch as an active normal task once committed so we can write a change notice as a followup.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

Regarding #2, in this issue renaming the method or property is out of scope, because it would affect significantly more code than is touched by this patch. See #1856630: [Change notice] [META] Rename Views methods to core standards.

There is an entire postponed meta issue for renaming methods, but not for properties yet, so I'll file that one as well and we can take care of the rename there at the appropriate phase of the release cycle.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: Unassigned » tim.plunkett
Status: Needs review » Needs work

StylePluginBase::$rendered_fields is never actually declared, and should be protected. Working on that next.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: tim.plunkett » Unassigned
Status: Needs work » Needs review
FileSize
interdiff.txt4.22 KB
vdc-1861838-7.patch7.7 KB

Okay, cleaned that up.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

#7: vdc-1861838-7.patch queued for re-testing.

dawehner’s picture

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

This is RTBC once it is green.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Looks good, I love it when we can just remove whole methods :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, vdc-1861838-7.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
drupal-1861838-12.patch7.75 KB

I really like the @see at the property.

This is just a rerole.

damiankloip’s picture

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

I think this one is good to go now this is green. Nice docs, good tidy up.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Nice clean-up. Committed to 8.x. Thanks.

Status: Fixed » Closed (fixed)

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

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Title: Clean up the various Views field rendering methods » Change notice: Clean up the various Views field rendering methods
Project: Drupal core » Views (for Drupal 7)
Version: 8.x-dev » 8.x-3.x-dev
Component: views.module » Documentation
Status: Closed (fixed) » Active
Chris Matthews’s picture

Chris Matthews CreditAttribution: Chris Matthews as a volunteer and at City of Oaks Design commented
Project: Views (for Drupal 7) » Drupal core
Version: 8.x-3.x-dev » 8.7.x-dev
Component: Documentation » views.module
Issue summary: View changes

For more information as to why this issue was moved to the Drupal core project, please see issue #3030347: Plan to clean process issue queue

Chris Matthews’s picture

Chris Matthews CreditAttribution: Chris Matthews as a volunteer and at City of Oaks Design commented
Project: Drupal core » Views (for Drupal 7)
Version: 8.7.x-dev » 7.x-3.x-dev
Component: views.module » Code
Status: Active » Closed (outdated)

Moving back to the contributed Views issue queue and closing as outdated per https://www.drupal.org/project/views/issues/3030347#comment-13023447