Missing @return tags in function/method comments in the core/modules folder

1. +++ b/core/modules/contextual/src/Plugin/views/field/ContextualLinks.php
   @@ -87,15 +87,7 @@ public function preRender(&$values) {
   -   * @see contextual_preprocess()
   -   * @see contextual_contextual_links_view_alter()
   

   It would be nice to not loose this particular bit of information

2. +++ b/core/modules/views/src/Plugin/views/row/OpmlFields.php
   @@ -212,6 +212,8 @@ public function render($row) {
       *   The ID assigned to the required field in the display.
   +   * @return string
   
   +++ b/core/modules/views/src/Plugin/views/row/RssFields.php
   @@ -197,6 +197,8 @@ public function render($row) {
   +   * @return \Drupal\Component\Render\MarkupInterface|null
   +   *   The output of the field, or NULL if it was empty.
       */
   

   Can we have a new line in between and why does one have a different return value than the other one even if its the same method?

I agree this is rc eligible.

Thanks! Needs a bit of work though:

1. +++ b/core/modules/views/src/Plugin/views/row/OpmlFields.php
   @@ -212,6 +212,8 @@ public function render($row) {
       *   The ID assigned to the required field in the display.
   +   * @return string|null
   +   *   The output of the field, or NULL if it was empty.
   

   There should be a blank line before a @return

   I also don't understand this documentation -- "output" of a field? You mean the rendered field value?

   And I doubt it is really a string that is returned, wouldn't it be a render array or something like that? Just seems wrong.

   Anyway I have no idea what "output" means.

2. +++ b/core/modules/views/src/Plugin/views/row/RssFields.php
   @@ -197,6 +197,8 @@ public function render($row) {
       *   The ID assigned to the required field in the display.
   +   * @return \Drupal\Component\Render\MarkupInterface|null
   

   Need blank line before @return

3. +++ b/core/modules/views/src/Plugin/views/row/RssFields.php
   @@ -197,6 +197,8 @@ public function render($row) {
   +   *   The output of the field, or NULL if it was empty.
   

   Again, what is "output"?

When you make a new patch in the future, please include an interdiff file. Thanks! Well, it's probably not all that necessary on such a small patch, but still a good idea.

Anyway... I looked carefully at this patch and went and read the code. The docs in OpmlFields and RssFields are totally wrong about the return values. Please go read the code. Those methods do not return NULL, MarkupInterface, or an array.

@ jhodgdon

The following is the function in the 'OpmlFields.php' .

public function getField($index, $field_id) {
    if (empty($this->view->style_plugin) || !is_object($this->view->style_plugin) || empty($field_id)) {
      return '';
    }
    return (string) $this->view->style_plugin->getField($index, $field_id);
  }

Is the return is showing return (string) $this->view->style_plugin->getField($index, $field_id); Isn't that returning the String of rendered field values ?

Correct me if I am wrong!

@jhodgdon

I understand the function public function getField returns string.

@dawehner

Is the same functionpublic function getField is using in both RssFields.php and OpmlFields.php.
I believe its same because both are trying to retrieve the views field value. If it is , then there is (string) is missing

--- a/core/modules/views/src/Plugin/views/row/RssFields.php
+++ b/core/modules/views/src/Plugin/views/row/RssFields.php
@@ -197,12 +197,15 @@ public function render($row) {
    *   The index count of the row as expected by views_plugin_style::getField().
    * @param $field_id
    *   The ID assigned to the required field in the display.
+   *
+   * @return string
+   *   String of rendered field values.
    */
   public function getField($index, $field_id) {
     if (empty($this->view->style_plugin) || !is_object($this->view->style_plugin) || empty($field_id)) {
       return '';
     }
-    return $this->view->style_plugin->getField($index, $field_id);
+    return (string) $this->view->style_plugin->getField($index, $field_id);
   }
 

As of my understanding, in both RssFields.php and OpmlFields.php files, it @retruns string and the doc comment like follows

    *   The index count of the row as expected by views_plugin_style::getField().
    * @param $field_id
    *   The ID assigned to the required field in the display.
+   *
+   * @return string
+   *   Rendered field.
    */
   public function getField($index, $field_id) {

Please find my final patch for review

+++ b/core/modules/views/src/Plugin/views/row/RssFields.php
@@ -197,12 +197,15 @@ public function render($row) {
       return '';
     }
-    return $this->view->style_plugin->getField($index, $field_id);
+    return (string) $this->view->style_plugin->getField($index, $field_id);

This is a documentation issue but you are changing code. That is not totally wrong, but it will have implications. Let's see what others say.

@cilefen
I created separate issue , as a child of this issue. Please find the following link for that
https://www.drupal.org/node/2597862

@here

I create new issue for the code change , which is https://www.drupal.org/node/2597862

We are keeping this issue only for documentation, that will be make things clear.

Please review the latest patch only having the documentation changes.

Right. This patch is correct for changing the docs so it says that it returns a string. You could have put a "the" in there but it's OK as is, at least correct and clear.

I have no comment about changing the code, but if you do that on another issue, please also make sure to change the corresponding documentation.

Thanks! New patch even more RTBC. ;) Hopefully the test bot will eventually stop trying to test the old patches. ?!?

+++ b/core/modules/views/src/Plugin/views/row/RssFields.php
@@ -197,6 +197,9 @@ public function render($row) {
+   *
+   * @return string
+   *   The rendered field value.

See the return value of StylePluginBase::getField() this returns \Drupal\Component\Render\MarkupInterface|null

Yes the base class method does, but not this one if you look at its code.

OK. Verified this is now correct return value on the Rss class. @alexpott - thanks for catching that. @rakesh.gectcr thanks for patching!

+++ b/core/modules/contextual/src/Plugin/views/field/ContextualLinks.php
@@ -87,15 +87,7 @@ public function preRender(&$values) {
-   * Overrides \Drupal\views\Plugin\views\field\FieldPluginBase::render().
-   *
-   * Renders the contextual fields.
-   *
-   * @param \Drupal\views\ResultRow $values
-   *   The values retrieved from a single row of a view's query result.
-   *
-   * @see contextual_preprocess()
-   * @see contextual_contextual_links_view_alter()
+   * {@inheritdoc}

So by switching this to {@inheritdoc}, we lose the two @see. Have we adopted a standard for this yet? Previously, we would copy the entire docblock (which is what HEAD does) and add the additional information. We could leave it as is in HEAD. Other options are:

/**
 * {@inheritdoc}
 * 
 * @see blah
 */

(which appears to work in IDEs, at least PHPStorm, but I am not sure api.d.o supports it yet)

or adding elsewhere, e.g. as inline comments at the beginning of the method, which I think is non-ideal since it makes it less visible.

I think maybe there is an existing issue for this? Especially now that #2502621: Replace implement notes with inheritdoc tag is in, it would be good to adopt a standard for the scenario where we want to inherit most, but not all of the documentation. @jhodgdon, thoughts?

There is NO documentation parser out there that supports inheritdoc that means "Inherit and then allow overrides". It is just not feasible. We have an issue about that but it's really a "won't fix". Some docs parsers support "inheritdoc means inherit everything", like we do. Some have it mean "inherit this little piece of something depending on where it's placed", which is pretty limited and not how we're using it. No one does the really really really really complicated logic of "inherit everything and then allow overrides". It is in the category of unicorns.

This comes up *all the time* and there are doc blocks that are trying to do this in Core and it just will not work.

Anyway... rant aside... If we feel that we really need those two @see items, we cannot use @inheritdoc. We can, however, say something like "See .... for documentation of parameters and return value" rather than duplicating the docs completely, although it is not all that wonderful to do that because it means the docs are not there if someone needs them.

the issue about partial inheritdoc is #1994890: Allow {@inheritdoc} and additional documentation

Well, there is the one in phpstorm which is certainly using some form of documentation parser, isn't it?

@xjm, @jhodgdon, @YesCT , @dawehner

Well, Please give me a conclusion, So that i can go forward and work accordingly.

Right now we do not allow @inheritdoc with additions. So let's just leave this documentation block unchanged in this patch. Thanks!

@jhodgdon

I removed the {@inheritdoc}. Please find the attached patch and interdiff file

Thanks!

+++ b/core/modules/views/src/Plugin/views/row/RssFields.php
@@ -197,6 +197,9 @@ public function render($row) {
+   * @return \Drupal\Component\Render\MarkupInterface|null|string
+   *   The rendered field value.

... is unfortunate

  public function getField($index, $field_id) {
    if (empty($this->view->style_plugin) || !is_object($this->view->style_plugin) || empty($field_id)) {
      return '';
    }
    return $this->view->style_plugin->getField($index, $field_id);
  }

So we return an empty string if there is no style plugin or if the field_id is empty (I wonder if this is even possible). And we return a NULL if the field is actually empty - otherwise we return a MarkupInterface object.

Anyhow this should be documented. See the documentation for StylePluginBase::getField() for a start of how to write this...

@alexpott
:D , I have done the changes according to your comment.

Good start but this needs some wording/grammar cleanup...

Also, use English in documentation. So instead of "field_id" say "field ID".

And we don't normally want to start a @return with "It returns". Leave that out.

And it shouldn't have [the rendered field value] in it... what is that for? Either use () (which is what you would use in English text), or leave out the [].

@jhodgdon

I have done the changes.

+++ b/core/modules/views/src/Plugin/views/row/RssFields.php
@@ -197,6 +197,10 @@ public function render($row) {
+   *   An empty string if there is no style plugin, or the field ID is empty.
+   *   And NULL if the field is actually empty otherwise MarkupInterface object.

Much better!

So... What does "if the field is actually empty" mean? The arguments here are $index and $field_id, so I am not sure what this means. I also wouldn't use the word "actually".

Then I would also suggest breaking up the second line (once it is clarified) into two sentences, and not starting with "And".

And finally, this isn't telling me what the MarkupInterface object contains (which is probably something about the rendered field value, right?).

So ... I think it should say something like:

NULL if .... If neither of these conditions apply, a MarkupInterface object containing ....

And you'll need to figure out what to substitute in for the .... parts.

Thanks!

Great! Very clear, and I have also verified it is correct. Thanks!

Committed 98e9354 and pushed to 8.0.x. Thanks!