core/includes/* , Add @return values at comment docblocks.

Hm. It seems like this may be a duplicate of another issue, but I'm not finding it at the moment.

Anyway, thanks! But...

1. +++ b/core/includes/schema.inc
   @@ -152,6 +152,8 @@ function drupal_uninstall_schema($module) {
   + * @return array
   

   If you are going to add an @return array line, you also need to add documentation to it.

2. +++ b/core/includes/unicode.inc
   @@ -71,7 +71,7 @@ function unicode_requirements() {
   + * @return resource
   

   This is not a valid type.

   See

   https://www.drupal.org/node/1354#types

Thanks! This needs some work though:

1. +++ b/core/includes/schema.inc
   @@ -152,6 +152,9 @@ function drupal_uninstall_schema($module) {
   + * @return array|mixed
   + *   Return the schema of a module.
   

   We do not normally start @return doc lines with "Return". Just say "The schema...".

   Also it isn't any old module whose schema is returned, so "a" should be "the".

2. +++ b/core/includes/unicode.inc
   @@ -71,7 +71,7 @@ function unicode_requirements() {
   + * @return object
     *   An XML parser object or FALSE on error.
   

   The @return type is incomplete here. It should probably be a specific type of object not generic "object" ? unsure... but it should also be |false according to the docs line.

Thanks for the patch! Hmmm....

1. +++ b/core/includes/schema.inc
   @@ -152,6 +152,9 @@ function drupal_uninstall_schema($module) {
   + * @return array|mixed
   + *   The schema of the module.
     */
    function drupal_get_module_schema($module, $table = NULL) {
   

   Um. Isn't it always an array? I don't think this function can ever return something other than an array.

2. +++ b/core/includes/unicode.inc
   @@ -71,8 +71,8 @@ function unicode_requirements() {
   + * @return resource|false
   

   What is "resource"? If it is a class, give the full namespace of the class. If it is an array, call it an array. If it's a generic PHP object, call it "object". But "resource" isn't something I recognize.

Hello, I have applied the last patch given by heykarthikwithu , thanks for the patch. Now I have added the changes that required.

1 . +++ b/core/includes/schema.inc
@@ -152,6 +152,9 @@ function drupal_uninstall_schema($module) {
  * @param string $table
  *   The name of the table. If not given, the module's complete schema
  *   is returned.
+ *
+ * @return array
+ *   The schema of the module.

2. +++ b/core/includes/unicode.inc
@@ -71,7 +71,7 @@ function unicode_requirements() {
  * @param $data
  *   The XML data which will be parsed later.
  *
- * @return
+ * @return object|false
  *   An XML parser object or FALSE on error.
  *
  * @ingroup php_wrappers

PLEASE make an interdiff file when you create a new patch on an issue that had a previous patch. Do not put the changes into a comment. Make an interdiff file and attach it. Thanks!

So... mostly looks good! But this return line is still not right:

+++ b/core/includes/unicode.inc
@@ -71,7 +71,7 @@ function unicode_requirements() {
+ * @return object|false
  *   An XML parser object or FALSE on error.

This is my fault -- I'm sorry about the previous review... I looked more carefully at the code this time. It turns out that this function is returning the output of the PHP xml_parser_create() function.

This is in fact a resource handle, not an xml object and not a generic object. See
http://php.net/manual/en/language.types.resource.php

So. It looks like we should actually say the return type is
resource|false

Also the docs line needs adjustment since this is not an "XML object" but a "resource handle for the XML parser".

I will add this to https://www.drupal.org/node/1354#types also because "resource" is a valid type to indicate.

Thanks! This one looks right to me.

Thanks @jhodgdon for the explanation in #16 and the update to https://www.drupal.org/node/1354#types; those comments helped me review this patch.

+++ b/core/includes/schema.inc
@@ -152,6 +152,9 @@ function drupal_uninstall_schema($module) {
+ * @return array
+ *   The schema of the module.

For complex array structures, we should generally explain what the structure of the array is or where it is defined. Looking at the code, it should say something like, "The complete schema of the module as defined in hook_schema(), or the schema for $table for the module."

(The complexity of the return value there suggests that really this should be two separate functions. But that's out of scope here; probably that refactor would be a part of moving these to the module handler service or a specific service for module schemata.)

I also noticed that the $table parameter is missing its (optional). I'd be okay with adding that small fix to the scope here since it's related to the return value that needs to be documented.

The other two return values in the patch look correct to me. Since checking and correcting return values for individual functions requires reviewing each closely, I think the scope of this issue makes sense. (I.e., it would unnecessarily complicate things to expand the scope to include fixing @return values across core.) So let's just clarify that return value for drupal_get_module_schema(). Thanks!

Thanks! For this review, I only looked at the interdiff file... It's mostly good but:

+++ b/core/includes/schema.inc
@@ -150,11 +150,12 @@
- *   The name of the table. If not given, the module's complete schema
+ *   (optional) Name of the table. If not given, module's complete schema
  *   is returned.

This needs "the" before "module's", which was there in the original before this patch. Please restore it. Thanks!

Hi jhodgdon,
Thanks for the review , improvements made as per your suggestion.

@Shreya, Interdiff file is important here.
As jhodgdon mentioned in #22. She reviewed only interdiff file.

I reworked on #22 and provided patch with interdiff file.

thanks ashish for the interdiff next time will give a patch with interdiff.

OK, these 3 random doc block changes look fine to me now. Thanks!

1. +++ b/core/includes/database.inc
   @@ -116,7 +116,7 @@ function db_query_range($query, $from, $count, array $args = array(), array $opt
   - * @return
   + * @return string
     *   The name of the temporary table.
   
   +++ b/core/includes/schema.inc
   @@ -150,8 +150,12 @@ function drupal_uninstall_schema($module) {
   + * @return array
   + *   The complete schema of the module as defined in hook_schema(),
   + *   or the schema for $table for the module.
   
   +++ b/core/includes/unicode.inc
   @@ -71,8 +71,8 @@ function unicode_requirements() {
   - * @return
   - *   An XML parser object or FALSE on error.
   + * @return resource|false
   + *   A resource handle for the XML parser or FALSE on error.
   

   Here we have a mix of new and changed documentation with a simple addition of type hints. Which is a mixing of scope.

2. +++ b/core/includes/schema.inc
   @@ -150,8 +150,12 @@ function drupal_uninstall_schema($module) {
   - *   The name of the table. If not given, the module's complete schema
   + *   (optional) Name of the table. If not given, the module's complete schema
   

   This is out of scope as it is not about return values.

See #2608536-14: Some fixes for 'optional' parameter in core/include for a recent comment about scoping issues.

I think this issue just needs to be closed as Won't Fix for scope problems. See
https://www.drupal.org/core/scope