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...

Files: 
CommentFileSizeAuthor
#11 2034979-11-views_resultrow_typehint.patch27 KBderhasi
PASSED: [[SimpleTest]]: [MySQL] 59,291 pass(es).
[ View ]
#11 interdiff.txt1.19 KBderhasi
#8 2034979-8-views_resultrow_typehint.patch27 KBderhasi
PASSED: [[SimpleTest]]: [MySQL] 59,235 pass(es).
[ View ]
#8 interdiff.txt19.55 KBderhasi
#4 2034979-4-views_resultrow_typehint.patch25.6 KBderhasi
PASSED: [[SimpleTest]]: [MySQL] 58,580 pass(es).
[ View ]

Comments

damiankloip’s picture

Status:Postponed» Active

Good to go

Crell’s picture

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

derhasi’s picture

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

Applied summary template and now going to work on it.

derhasi’s picture

Status:Active» Needs review
StatusFileSize
new25.6 KB
PASSED: [[SimpleTest]]: [MySQL] 58,580 pass(es).
[ View ]

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

Assigned:derhasi» Unassigned
derhasi’s picture

Issue summary:View changes

Applying summary template

dawehner’s picture

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

  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

StatusFileSize
new19.55 KB
new27 KB
PASSED: [[SimpleTest]]: [MySQL] 59,235 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Good work!

damiankloip’s picture

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

StatusFileSize
new1.19 KB
new27 KB
PASSED: [[SimpleTest]]: [MySQL] 59,291 pass(es).
[ View ]

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

derhasi’s picture

Status:Needs work» Needs review
damiankloip’s picture

Status:Needs review» Reviewed & tested by the community

Great, thank you!

damiankloip’s picture

Issue summary:View changes

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

catch’s picture

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

Issue summary:View changes

Updated "Remaining tasks"