Updated: Comment #13

Problem/Motivation

To provide a better interface, field handler methods should typehint the result row values (Drupal\views\ResultRow).

Proposed resolution

Typehinting params of field handler methods using Drupal\views\ResultRow and adjust docblocks appropriately:

  • render()
  • advancedRender()
  • renderLink()
  • getValue()
  • theme()

Remaining tasks

None.

User interface changes

None.

API changes

None.

Original report by @damiankloip

After #2034947: [Change notice] Change view results to use a classed object is in, we can typehint all the places this is used. Field handler methods render, advancedRender, render_link, getValue, maybe more...

CommentFileSizeAuthor
#11 2034979-11-views_resultrow_typehint.patch27 KBderhasi
#11 interdiff.txt1.19 KBderhasi
#8 2034979-8-views_resultrow_typehint.patch27 KBderhasi
#8 interdiff.txt19.55 KBderhasi
#4 2034979-4-views_resultrow_typehint.patch25.6 KBderhasi

Comments

damiankloip’s picture

damiankloip commented
Status: Postponed » Active

Good to go

Crell’s picture

Crell commented
Issue tags: +Needs issue summary update

This needs more precise directions for a novice issue. (Hello from Drupal Camp Costa Rica! Pura Vida!)

derhasi’s picture

derhasi commented
Assigned: Unassigned » derhasi
Issue tags: -Needs issue summary update

Applied summary template and now going to work on it.

derhasi’s picture

derhasi commented
Status: Active » Needs review
StatusFileSize
new 2034979-4-views_resultrow_typehint.patch25.6 KB

Attached is the patch for typehinting ResultRow in all views field plugins. It's quite a big patch, as there where a lot of docblocks not properly provided.

derhasi’s picture

derhasi commented
Assigned: derhasi » Unassigned
derhasi’s picture

derhasi commented
Issue summary: View changes

Applying summary template

dawehner’s picture

dawehner
Primary language German
commented

Wow this was a big patch. Thank you for working on that!!

  1. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/Link.php
@@ -51,6 +51,17 @@ public function render(ResultRow $values) {
   protected function renderLink($data, ResultRow $values) {

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/LinkApprove.php
@@ -24,6 +24,17 @@ public function access() {
   protected function renderLink($data, ResultRow $values) {

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/LinkDelete.php
@@ -24,6 +24,17 @@ public function access() {
   protected function renderLink($data, ResultRow $values) {

    It would maybe make sense to typehint CommentInterface on the common base class (the link handler for comments) so you get better autocompletion in this single function but yeah this is maybe out of scope. 

  2. +++ b/core/modules/node/lib/Drupal/node/Plugin/views/field/Link.php
@@ -55,6 +55,17 @@ public function render(ResultRow $values) {
+   * @param ResultRow $values

+++ b/core/modules/node/lib/Drupal/node/Plugin/views/field/LinkEdit.php
@@ -21,7 +21,15 @@
+   * @param ResultRow $values

    Let's use the full namespace here as well.

damiankloip’s picture

damiankloip commented 
  1. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/LinkDelete.php
@@ -24,6 +24,17 @@ public function access() {
+   *   Returns string for the link text.

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/LinkEdit.php
@@ -37,6 +37,17 @@ public function buildOptionsForm(&$form, &$form_state) {
+   *   Returns string for the link text.

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/LinkReply.php
@@ -24,6 +24,17 @@ public function access() {
+   *   Returns string for the link text.

    'Returns a string for the link text' 

  2. +++ b/core/modules/contextual/lib/Drupal/contextual/Plugin/views/field/ContextualLinks.php
@@ -68,6 +68,9 @@ public function preRender(&$values) {
+   *   The values retrieved from the database.

    Can this be more general?

    Just the values from a row of view results or something? (I have nothing that good right now! :))

Otherwise, generally looking pretty good.

derhasi’s picture

derhasi commented
StatusFileSize
new interdiff.txt19.55 KB
new 2034979-8-views_resultrow_typehint.patch27 KB

It would maybe make sense to typehint CommentInterface on the common base class (the link handler for comments) so you get better autocompletion in this single function but yeah this is maybe out of scope.

@dawehner, yes, should make sense, but I think it is out of scope for this issue.

dawehner, damiankloip: I implemented your proposed changes (see interdiff.txt).

dawehner’s picture

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

Good work!

damiankloip’s picture

damiankloip commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/Counter.php
@@ -8,6 +8,8 @@
+

    Extra line. 

  2. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/FieldPluginBase.php
@@ -1591,8 +1591,14 @@ protected function documentSelfTokens(&$tokens) { }
+   * @param \Drupal\views\ResultRow

    No variable name.

Sorry.. :/

*hides*

derhasi’s picture

derhasi commented
StatusFileSize
new interdiff.txt1.19 KB
new 2034979-11-views_resultrow_typehint.patch27 KB

@damiankloip, thanks, nothing to hide ;) You are totally right, so here's the fix.

derhasi’s picture

derhasi commented
Status: Needs work » Needs review
damiankloip’s picture

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

Great, thank you!

damiankloip’s picture

damiankloip commented
Issue summary: View changes

Updating method names found. Remaining task added to decide on docblock change.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Updated "Remaining tasks"