Fix function docblocks in WebTestBase

@throws patch already coming with issue https://www.drupal.org/node/2595999

@Here the attached patch having
Figure out the correct return types for each method.[done]
Add missing @throws tags.[done]
Roll a patch to fix the docblocks[done]

@here

It is related issue to https://www.drupal.org/node/2595999#comment-10483912

Please see the #points 8,9 & 10, @jhodgdon has mentioned.

So i am changing the issue to update the doc comment without @throws

I am attaching the patch that updating the comment docs without @throws

Noticed some inaccuracies in the @return tags, so taking this patch a bit further.

@kekkis,

Can you please leave the review comment here, Something like where you feeling inaccuracies.

I'm working on a new patch as we speak, that will show you what I mean. Will include an interdiff as well.

Here. Fixed a few return types, even added one IIRC. The most notable changes involve return values of Json::decode(), which may return whatever json_decode() returns.

Actually, according to https://www.drupal.org/coding-standards/docs#types (which I discovered just now) there are still opportunities for improvement here. Specifically, one could use the notation string[] at some places.

Thanks for the issue/patch!

Mostly good; a few things to address:

1. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -238,8 +238,8 @@ function __construct($test_id = NULL) {
   +   * @return \Drupal\node\NodeInterface|bool
   +   *   A node entity matching $title, or FALSE if a node cannot be found.
   

   In this case, use "false" instead of "bool", since at least according to the docs line, TRUE cannot be returned, only FALSE.

   I've marked some other places like this in the patch but not all of them, by the way.

2. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -352,8 +352,9 @@ protected function drupalCreateContentType(array $values = array()) {
   +   *   The renderable view array.
       * @see drupal_render()
   

   We usually leave a blank line between @return and @see, although this may not be a strict standard.

   Also rather than "renderable" we usually call these things "render arrays".

3. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -470,8 +471,8 @@ protected function assertNoBlockAppears(Block $block) {
   +   * @return array|bool
   

   See above note on bool -> false

4. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -470,8 +471,8 @@ protected function assertNoBlockAppears(Block $block) {
   +   *   The result from the xpath query or FALSE if parsing the underlying xpath query fails.
   

   This line exceeds 80 characters.

   Also I think it could use a comma before the "or".

5. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -557,6 +558,14 @@ protected function drupalGetTestFiles($type, $size = NULL) {
   +   *   The difference of the $file1 and $file2.
   

   What does that mean? This is a comparison, presumably for use in a sort operation? So I suspect (not having looked at the code) that it would return 1, 0, or -1 depending on if the first or second file is "greater than" the other, meaning... well you'd need to look at the code.

   So I would say something like:

   Which file should come first, based on ....

   or something like that.

   And make sure it is clear what will cause it to return 1 and 0 and -1 (or whatever the return values actually are).

6. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -624,6 +633,9 @@ protected function drupalLogin(AccountInterface $account) {
   +   *   TRUE if the user is logged in else FALSE.
   

   Needs grammar/punctuation update...

   "else" => "; otherwise, "

1. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1303,15 +1315,15 @@ protected function curlInitialize() {
   +   * @return string
   

   There may be a possibility of this returning FALSE or something else as well? Please check on this in the code.

2. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1574,7 +1589,7 @@ protected function drupalGetJSON($path, array $options = array(), array $headers
   -   * @return mixed
   +   * @return string
   

   I suspect this is actually "mixed" since the format is variable?

3. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1703,6 +1718,9 @@ protected function drupalGetXHR($path, array $options = array(), array $headers
   +   * @return string
   +   *   The content.
       */
      protected function drupalPostForm($path, $edit, $submit, array $options = array(), array $headers = array(), $form_html_id = NULL, $extra_post = NULL) {
   

   always string really? And what content?

4. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -2089,7 +2107,7 @@ protected function drupalProcessAjaxResponse($content, array $ajax_response, arr
   +   * @return string
   

   really never NULL or FALSE or something?

5. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -2210,7 +2228,7 @@ protected function cronRun() {
   +   * @return string|bool
       *   Either the new page content or FALSE.
   

   Yeah see this one should be string|false, and I think all those other get/post methods can also be string|false right?

6. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -2273,8 +2291,9 @@ protected function drupalHead($path, array $options = array(), array $headers =
   +   *   Whether the submit element value matches
   +   *   a valid submit input in the form.
   

   Too short wrapping - please make the lines closer to 80 characters

7. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -2741,8 +2760,8 @@ protected function assertUrl($path, array $options = array(), $message = '', $gr
   -   *   Assertion result.
   +   * @return bool
   +   *   TRUE if assertion result found else FALSE.
   

   I think the previous doc line of "Assertion result" is actually better than the new text. I'm not saying the text couldn't be improved, but... maybe not like this... the previous text is clearer and more concise. Probably just leave it as it was.

8. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -2910,7 +2929,7 @@ protected function verboseEmail($count = 1) {
   +   * @return Request
   

   When you put a class in a @return type, include the namespace.

9. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -2959,7 +2978,7 @@ protected function prepareRequestForGenerator($clean_urls = TRUE, $override_serv
   -   *   An absolute URL stsring.
   +   *   An absolute URL string.
   

   Nice. :)

You did not really address my comments about the return values of the drupalGet* and drupalPost* functions.

Either address them by fixing the patch, or explain why you didn't address them.

@kekkis

Can you work on the changes mentioned by @jhodgdon

@jhodgdon

I have worked on your points

1. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1303,15 +1319,15 @@ protected function curlInitialize() {
   +   * @return string
   

   I have asked repeatedly to please look at the return values of the drupalGet* and drupalPost* functions. This has *still* not been done. I am pretty sure these functions can also return a FALSE value, and this is NOT documented in the patch.

   If you are going to write PHP documentation for return values, you need to read the PHP code that you are documenting *carefully* and check carefully what the return value is. Especially after someone reviewing your patch points out that she thinks the return docs are wrong.

   Then you need to either fix the patch, or if my review comment is wrong, you need to respond to it in a comment by posting some of the code in the function and an explanation of why your patch is correct.

   If you need help in making a fix to the patch, or in analyzing the code, please ask for help. There are mentoring hours when you can always get help with patches, and even outside of mentoring hours, if you are in #drupal-contribute IRC channel and ask for advice or help, you can probably find someone to help. You might also find
   https://www.drupal.org/node/1354#types

   I have asked repeatedly for this change to happen and it has not happened. I feel that you are not respecting the time and effort I have been spending reviewing your patches carefully.

   So... I stopped at this line of the patch and didn't read any more of it. But I also found some things to fix higher up in the patch before I stopped looking at it.

2. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -352,7 +352,9 @@ protected function drupalCreateContentType(array $values = array()) {
   +   *   The render arrays.
   

   Please change this to:

   A render array for [whatever it is for].

   Also just below here, please take out the @see drupal_render().

3. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -470,8 +472,9 @@ protected function assertNoBlockAppears(Block $block) {
   +   *   The result from the xpath query, or FALSE if parsing the underlying
   

   The correct capitalization is:

   XPath

   Please fix this in documentation and comments throughout the file.

4. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -507,7 +510,7 @@ protected function findBlockInstance(Block $block) {
   +   * @return array
       *   List of files in public:// that match the filter(s).
   

   Do not use @return array unless you do not know what type of things are in the array.

   You want to do something like
   @return string[]
   @return int[]
   @return \Some\Class\Name[]
   etc.

5. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -557,6 +560,16 @@ protected function drupalGetTestFiles($type, $size = NULL) {
   +   * @param string $file1
   +   *   The first file.
   +   * @param string $file2
   +   *   The second file.
   

   Look at the code. These are not strings.

6. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -557,6 +560,16 @@ protected function drupalGetTestFiles($type, $size = NULL) {
   +   *   The return value < 0 the $file1 is less than $file2,
   +   *   > 0 the $file1 is greater than $file2,
   +   *   or 0 both $file1 and $file2 are equal.
   

   This is still wrong.

   Files are not "less than" or "greater than" other files.

   You need to be more specific about explaining the conditions.

   Also please use English words/phraing, like "The return value is an integer that is less than zero" rather than "The return value < 0".

7. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -624,6 +637,9 @@ protected function drupalLogin(AccountInterface $account) {
   +   *   TRUE if the user is logged in otherwise FALSE.
   

   Add a ; after "in".

Added void return docblocks

@kaushalkishorejaiswal Please provide the interdiff files also

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -223,6 +223,8 @@
+   * ¶

Please remove all trailing whitespace.

Why did you add

@return void

doc blocks? We do not do that. If there is no return value for a function, we do not put in @return.

Also I cannot see that any of my previous review comments were addressed, although with no interdiff file it is hard to tell... Anyway we should probably go back to the patch in #34 because we do NOT want @return void doc blocks.

@jhodgdon

I am rolled back the patch on #34.

1) About this I have added the

 @return string|false
   *   The content returned from the call to curl_exec(), or FALSE.

Reviewing the interdiff...

1. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -354,9 +354,7 @@
   +   *   A render array for [built view of an entity].
   

   Sorry. You've taken one of my review comments a bit too literally. When I said:
   A render array for [whatever it is for].
   I didn't mean you should put anything in [] brackets! :)

2. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -561,15 +559,17 @@
   +   * @param resource $file1
   

   What is "resource"? Don't make up return types. Use the actual class/interface name, with the namespace, if there is one. Otherwise, use "array" for an array, or "object" for a PHP standard object.

   See
   https://www.drupal.org/node/1354#types
   for more information.

3. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -561,15 +559,17 @@
   +   *   The return value is an integer that is less than zero when the
   +   *   $file1's name is less than $file2's name, the return value is an integer
   +   *   that is greater than zero when the $file1's name is greater than
   +   *   $file2's name, or the return value is zero when both
   +   *   $file1's and $file2's names are equal.
   

   This is not accurate at all. The file sizes are compared first, and only if the sizes are the same are the file names compared.

   I would also not advise saying that a file's name is "less than" another -- this is ambiguous. Say it comes before/after in alphabetical order or something like that.

   I think this return value documentation would probably be easier to read, anyway, if it were formatted as a list. Something like:

   An integer suitable for sorting the files, with value:
   - first condition and result
   - second condition and result
   - etc.

4. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1327,8 +1327,8 @@
   +   * @return string|false
   +   *   The content returned from the call to curl_exec(), or FALSE.
   

   Good. Now this needs to be done for
   
   ALL OF THE get* and post* methods on this class
   
   not just this one.

   I have said this about 5 times in previous comments. ALL of the get* and post* methods need to be checked over. I believe all of them can return FALSE -- I could be wrong, but if so you need to show code samples for the methods that cannot return FALSE and document all of the other ones properly.

   Really, how many times do I need to say the same thing in a review? Please do not submit another patch without checking over all of these methods.

   Sorry for shouting here but this is getting really annoying... Whoever makes the next patch, you'll need to read the code in all of the get* and post* methods on this class and carefully figure out what they can return. Documenting return values is only valuable if it's accurate, and to be accurate sometimes you really need to read the code carefully. If you have time, please do it. If you don't, please find another issue to work on that is easier -- there are plenty out there that can be done without as much effort.

@rakesh.gectcr Please follow all the comment threads written for each issue.

Don't make issues for small errors, please concatenate them all and make one issue.
It will helpful for reviewer and tester to review and test the patch.

On the other hand if you are not solving issue or unable to solve it please unassigned yourself from the issue.
So that anyone else can pick this issue and can work on it.

I am working on this.

Changes done in patch.
As mentioned in comment #42.
1. Changed to "A render array for view entity."
2. Changed to object.
3. Changed as mentioned.
4. Changed all of the get* and post* methods on this class.

Um. The last patch here has too many problems to even review. Please go back to #40 and start over. Sorry.

Since simpletest was removed from core in #3110862: Remove simpletest module from core moving this to the Simpletest Module.