This is a sub-issue of #1800046: [META] Add missing type hinting to core docblocks, fix Drupal.Commenting.FunctionComment.Missing* focused on correctly adding @param and @return type hinting to the System module.

Documentation patches that include type hinting are time consuming to both review and commit because one must dig into the actual code to confirm that the type hints are both correct and complete. Hence, please be patient and try to limit type hint patches to covering only a limited number of docblocks (10-15 as a guess).

How to review this issue:

You can use phpcs this way to check for remaining @param and @return hint errors:

phpcs --standard="Drupal" --extensions="module/php,inc/php,php" --report-csv --ignore="core/modules/system/tests,core/modules/system/src/Tests" core/modules/system/ | grep -F $'Missing param\nReturn type missing'

Note that this uses --ignore to exclude test files, since those are handled in another issue: #2651280: Add missing type hinting to System module TEST docblocks

Related sprint issues:

Sprint Topic Sub Issue
#1518116: [meta] Make Core pass Coder Review #1533400: Make system module (except tests subdirectory) pass Coder Review
#1310084: [meta] API documentation cleanup sprint #1326664: Clean up API docs for system module (excluding subdirectories)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lars Toomre’s picture

Status: Active » Needs review
FileSize
15.14 KB

Attached is a patch that was rejected in #1326664-74: Clean up API docs for system module (excluding subdirectories) for the inclusion of type hinting of @param and @return directives.

Lars Toomre’s picture

Here is an initial patch that adds type hinting to the three *.api.php. This patch is incomplete because there are some missing @param directives being added via #1326664: Clean up API docs for system module (excluding subdirectories).

There are also a couple of places where I was unsure of what the type hint should be. Those cases are indicated by '???'.

Finally, where missing, those function parameters that are arrays had the type hint added to the function declaration as well.

sun’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/system.api.php
@@ -548,16 +548,16 @@ function hook_page_build(&$page) {
- * @param $original_map
+ * @param ??? $original_map
  *   The path argument map, as contained in $path.
...
+function hook_menu_get_item_alter(array &$router_item, $path, $original_map) {

array

+++ b/core/modules/system/system.api.php
@@ -3883,12 +3883,12 @@ function hook_countries_alter(&$countries) {
- * @param $menu_site_status
+ * @param ??? $menu_site_status
  *   Supported values are MENU_SITE_OFFLINE, MENU_ACCESS_DENIED,

integer

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
54.96 KB

Thanks @sun. Here is a revised patch that eliminates the ??? items.

RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

Just touches System module's api.php docs. Looks good.

webchick’s picture

Component: system.module » documentation
Assigned: Unassigned » jhodgdon

Moving to Jennifer's queue.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Needs work

@Rob Loach, I have to say that your review comment did not give me confidence that you had actually reviewed all the changes carefully to verify that they are all correct... these type hinting patches require looking at the function code in most cases.

So I gave it a thorough review, and found a couple of issues -- please provide an interdiff along with the next patch so I don't have to review the whole thing again! :)

a) hook_file_download()'s return value can also be NULL, so this isn't quite right:

- * @return
+ * @return int|array
  *   If the user does not have permission to access the file, return -1. If the

b) Since we need another patch anyway, could you also fix this:

- * @param $module
+ * @param array $module
  *   An array of modules about to be enabled.
  */
-function hook_modules_preenable($modules) {
+function hook_modules_preenable(array $modules) {

The parameter is apparently $modules not $module

c) hook_query_alter() and hook_query_TAG_alter():

- * @param $query
+ * @param Drupal\Core\Database\Query\AlterableInterface $query
  *   A Query object describing the composite parts of a SQL query.

The type and the description are inconsistent. One says it's an object of type Query, and the other says it's an AlterableInterface. I have no idea which is correct.

d)

- * @param $menu_site_status
+ * @param integer $menu_site_status

Should be int not integer to comply with our data type standards.

e) And... One comment on this whole effort. Can you really say that this type of change is adding a lot of clarity to the documentation of Drupal?

- * @param $data
+ * @param array $data
  *   The associative array ...

I mean, the information is already there in the description that it's an array, and 95% of the changes in this patch are like this. I'm not against bringing this up to our standards, of course, but I was specifically requested to prioritize this by Lars and I just don't get why this is of higher priority than any of our other "bring this up to standards" patches... it seems of a similar nature to the other clean-up efforts. Again, I'm not against this effort but you haven't convinced me it's a silver bullet or higher priority.

bleen’s picture

Status: Needs work » Needs review
FileSize
55.07 KB
1.91 KB

I didnt do a separate review of this ... just made the adjustments suggested by jhodgman in #8

Lars Toomre’s picture

Thanks @bleen18. It would be great if you also could do a review. It will help in learning to roll the other type hinting patches.

I will try to review this latest patch later today.

Status: Needs review » Needs work

The last submitted patch, docblock-system-module-1488712.patch, failed testing.

bleen’s picture

Status: Needs work » Needs review

#4: 1800330-4-th-system-api.patch queued for re-testing.

bleen’s picture

I just did a carefull diff between the patches in #4 and #9 and nothing has changed outside of a docblock... so how can one fail tests and one pass?

bleen’s picture

bleen’s picture

ok .. must have been testbot having a brain fart ... #9 passed and is reviewable

eojthebrave’s picture

Issue summary: View changes
Status: Needs review » Needs work

Patch in #9 is pretty old and no longer applies cleanly so it'll need to be re-rolled.

error: patch failed: core/modules/system/language.api.php:64
error: core/modules/system/language.api.php: patch does not apply
error: patch failed: core/modules/system/system.api.php:49
error: core/modules/system/system.api.php: patch does not apply
error: patch failed: core/modules/system/theme.api.php:103
error: core/modules/system/theme.api.php: patch does not apply

Mile23’s picture

romina.nayak’s picture

Status: Needs work » Needs review
FileSize
15.86 KB

Rerolled for drupal 8.0.0-beta4.

Status: Needs review » Needs work

The last submitted patch, 18: docblock-system-module-1488712-18.patch, failed testing.

Mile23’s picture

Thanks, @romina.nayak. :-)

Next steps:

  • Click on [view] in the test result block and check out which tests are failing.
  • Figure out how to make those tests pass.

Its probably because this patch changes method signatures, and some overriding method needs to be modified to fit the new signature.

Mile23’s picture

Oh, and we always want to reroll against the latest commit in dev, not a release tag.

prashantgoel’s picture

HI @Mile23,

There is a lot of change while I was rerolling the last successful patch. Can you please guide me on this.

Thanks

Mile23’s picture

@prashantgoel: Here are some step-by-step instructions for doing a reroll: https://www.drupal.org/patch/reroll

The basic concept is that you make a separate branch sometime back in time when the patch applied. Then you do a rebase from the current branch. git will tell you if there are conflicts, and you can resolve them.

Thanks!

prashantgoel’s picture

Status: Needs work » Needs review
FileSize
47.92 KB

@Mile23: rebasing didn't worked as code is now changed to multiple files.

I am attaching a new patch with the changes. This patch is created using the last successful patch.

Thanks

Mile23’s picture

Status: Needs review » Needs work

Thanks for the reroll, @prashantgoel!

However, there are still a lot of 'Missing parameter type' errors reported by phpcs.

Also, I'm not entirely sure how you made this patch. It says it applies but the changes don't show for some files such as theme.api.php. Could you make another patch using git? Thanks.

rishikant05’s picture

Assigned: Unassigned » rishikant05
rishikant05’s picture

Assigned: rishikant05 » Unassigned
rishikant05’s picture

Assigned: Unassigned » rishikant05
Issue tags: +Needs reroll

patch in #24 is not applied.
Getting error, which i have pasted below.

Hunk #1 succeeded at 26 (offset 1 line).
error: while searching for:
* you will have to change the order of hook_form_alter() implementation in
* hook_module_implements_alter().
*
* @param $implementations
* An array keyed by the module's name. The value of each item corresponds
* to a $group, which is usually FALSE, unless the implementation is in a
* file named $module.$group.inc.
* @param $hook
* The name of the module hook being implemented.
*/
function hook_module_implements_alter(&$implementations, $hook) {
if ($hook == 'rdf_mapping') {
// Move my_module_rdf_mapping() to the end of the list.
// \Drupal::moduleHandler()->getImplementations()

error: patch failed: core/modules/system/module.api.php:62
error: core/modules/system/module.api.php: patch does not apply
Checking patch core/modules/system/system.api.php...
error: while searching for:
*
* See hook_schema() for details on the schema definition structure.
*
* @param $schema
* Nested array describing the schemas for all modules.
*
* @ingroup schemaapi
*/
function hook_schema_alter(&$schema) {
// Add field to existing schema.
$schema['users']['fields']['timezone_id'] = array(
'type' => 'int',

error: patch failed: core/modules/system/system.api.php:158
error: core/modules/system/system.api.php: patch does not apply

rishikant05’s picture

@prashantgoel: patch #24 is not applied.

I am attaching a new patch with the changes. This patch is created using patch #24.

Thanks

rishikant05’s picture

rishikant05’s picture

Assigned: rishikant05 » Unassigned
rishikant05’s picture

Status: Needs work » Needs review
FileSize
29.51 KB

Sorry I attached the same file 3 times by mistake in #29.

@prashantgoel: patch #24 is not applied.

I am attaching a new patch with the changes. This patch is created using patch #24.

Thanks

Status: Needs review » Needs work

The last submitted patch, 32: add-missing-type-hinting-to-system-module-docblocks#32.patch, failed testing.

prashantgoel’s picture

as mentioned in #25 this needs more work rather than just porting the patch from #24.

Thanks

no_angel’s picture

Assigned: Unassigned » no_angel
no_angel’s picture

Assigned: no_angel » Unassigned
Mile23’s picture

Version: 8.0.x-dev » 8.1.x-dev
Mile23’s picture

Issue tags: +Needs reroll

Patch no longer applies.

naveenvalecha’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
43.33 KB

Giving initial shot.
There are few left in Unit test cases which don't return anything b/c those are the helper dataproviders.
P.S. : Need a deeper review, where I was not sure so I used mixed,I'm fine to fix those in another shot.
Few are the pending b/c I was not suare about those what would be the parameter type, I can take care those in another shot.

Admins-MacBook-Pro% phpcs --standard="Drupal" --extensions="module/php,php" --report-csv core/modules/system | grep -F $'Missing param\nReturn type missing'
"/Applications/MAMP/htdocs/www/drupal81/core/modules/system/tests/src/Kernel/Extension/ModuleHandlerTest.php",89,6,error,"Missing parameter comment",Drupal.Commenting.FunctionComment.MissingParamComment,5,0
"/Applications/MAMP/htdocs/www/drupal81/core/modules/system/tests/src/Kernel/Extension/ModuleHandlerTest.php",89,6,error,"Missing parameter type",Drupal.Commenting.FunctionComment.MissingParamType,5,0
Mile23’s picture

Re: IRC: Yah, that's a lot of stuff to review. :-)

I made an issue for only the changes to tests: #2651280: Add missing type hinting to System module TEST docblocks

Here's the patch in #39 without the changes to tests. It's only 9k-ish.

naveenvalecha’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/system/system.module
    @@ -378,17 +378,17 @@ function system_theme_suggestions_field(array $variables) {
    + * @param string $callback
    

    The data type should be "callable"

  2. +++ b/core/modules/system/system.module
    @@ -827,7 +827,7 @@ function system_preprocess_block(&$variables) {
    + * @param string $form_element
    

    This should be an array as per its usage inside the function system_check_directory

One more thing, As @jhodgdon specified in one of the other similar issues that they are only documentation changes and they are 8.0.x material, so can we change the version to 8.0.x ?

jhodgdon’s picture

Version: 8.1.x-dev » 8.0.x-dev

Yes please. If these are only API documentation blocks they should be 8.0.x.

naveenvalecha’s picture

Status: Needs work » Needs review
FileSize
9.27 KB
1.03 KB

Addressing #41.1 and #41.2
phpcs reporting nothing.

Mile23’s picture

Issue summary: View changes
Status: Needs review » Needs work

Sorry, I borked the phpcs validation script one-liner. It doesn't look for .inc files, which are important. A little.

Updated issue summary to reflect this.

You can use phpcs this way to check for remaining @param and @return hint errors:

phpcs --standard="Drupal" --extensions="module/php,inc/php,php" --report-csv --ignore="core/modules/system/tests,core/modules/system/src/Tests" core/modules/system/ | grep -F $'Missing param\nReturn type missing'

Note that this uses --ignore to exclude test files, since those are handled in another issue: #2651280: Add missing type hinting to System module TEST docblocks

So there are files which still have errors, including system.admin.inc.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

snehalgaikwad’s picture

Status: Needs work » Needs review
FileSize
9.04 KB
8.22 KB

Adding patch for both 8.9.x and 9.1.x versions.

abhisekmazumdar’s picture

Thanks @snehalgaikwad for the patches.
Both the paches looks good to me. RTBC from me.

quietone’s picture

Status: Needs review » Closed (duplicate)
Related issues: +#3207949: Fix Drupal.Commenting.FunctionComment.MissingParamType

This work is now being done by sniff. The work here is now in #3207949: Fix Drupal.Commenting.FunctionComment.MissingParamType so closing this as a duplicate. I've identified credit to add over there, let me know if I got it wrong.