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:
Comment | File | Size | Author |
---|---|---|---|
#53 | 1800330-9.1.x-53.patch | 8.22 KB | snehalgaikwad |
#53 | 1800330-53.patch | 9.04 KB | snehalgaikwad |
#43 | interdiff-1800330-40-42.txt | 1.03 KB | naveenvalecha |
#43 | 1800330-42.patch | 9.27 KB | naveenvalecha |
#40 | 1800330_40.patch | 9.27 KB | Mile23 |
Comments
Comment #1
Lars Toomre CreditAttribution: Lars Toomre commentedAttached 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.
Comment #2
Lars Toomre CreditAttribution: Lars Toomre commentedHere 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.
Comment #3
sunarray
integer
Comment #4
Lars Toomre CreditAttribution: Lars Toomre commentedThanks @sun. Here is a revised patch that eliminates the ??? items.
Comment #5
RobLoachJust touches System module's api.php docs. Looks good.
Comment #6
webchickMoving to Jennifer's queue.
Comment #7
Lars Toomre CreditAttribution: Lars Toomre commented@Rob Loach Thank you for the review.
If you have the time, can you take a look at the other *.api.php type hinting patches that are a part of #1800046: [META] Add missing type hinting to core docblocks, fix Drupal.Commenting.FunctionComment.Missing*. Specifically, the following patches could use some attention:
- #1807160-1: Add missing type hinting to Aggregator module docblocks
- #1807674-1: Add missing type hinting to Block module docblocks
- #1800774-1: Add missing type hinting to menu_ui module docblocks
- #1800546-2: Add missing type hinting to Node module docblocks
- #1805340-2: Add missing type hinting to Simpletest module docblocks (excluding test base classes)
- #1807002-1: Add missing type hinting to Taxonomy module docblocks
- #1800174-3: Add missing type hinting to User module docblocks
Thank you again for your review above.
Comment #8
jhodgdon@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:
b) Since we need another patch anyway, could you also fix this:
The parameter is apparently $modules not $module
c) hook_query_alter() and hook_query_TAG_alter():
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)
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?
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.
Comment #9
bleen CreditAttribution: bleen commentedI didnt do a separate review of this ... just made the adjustments suggested by jhodgman in #8
Comment #10
Lars Toomre CreditAttribution: Lars Toomre commentedThanks @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.
Comment #12
bleen CreditAttribution: bleen commented#4: 1800330-4-th-system-api.patch queued for re-testing.
Comment #13
bleen CreditAttribution: bleen commentedI 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?
Comment #14
bleen CreditAttribution: bleen commented#9: docblock-system-module-1488712.patch queued for re-testing.
Comment #15
bleen CreditAttribution: bleen commentedok .. must have been testbot having a brain fart ... #9 passed and is reviewable
Comment #16
eojthebravePatch 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
Comment #17
Mile23Comment #18
romina.nayak CreditAttribution: romina.nayak commentedRerolled for drupal 8.0.0-beta4.
Comment #20
Mile23Thanks, @romina.nayak. :-)
Next steps:
Its probably because this patch changes method signatures, and some overriding method needs to be modified to fit the new signature.
Comment #21
Mile23Oh, and we always want to reroll against the latest commit in dev, not a release tag.
Comment #22
prashantgoel CreditAttribution: prashantgoel commentedHI @Mile23,
There is a lot of change while I was rerolling the last successful patch. Can you please guide me on this.
Thanks
Comment #23
Mile23@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!
Comment #24
prashantgoel CreditAttribution: prashantgoel commented@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
Comment #25
Mile23Thanks 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.
Comment #26
rishikant05 CreditAttribution: rishikant05 at Srijan | A Material+ Company commentedComment #27
rishikant05 CreditAttribution: rishikant05 at Srijan | A Material+ Company commentedComment #28
rishikant05 CreditAttribution: rishikant05 at Srijan | A Material+ Company commentedpatch 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
Comment #29
rishikant05 CreditAttribution: rishikant05 at Srijan | A Material+ Company commented@prashantgoel: patch #24 is not applied.
I am attaching a new patch with the changes. This patch is created using patch #24.
Thanks
Comment #30
rishikant05 CreditAttribution: rishikant05 at Srijan | A Material+ Company commentedComment #31
rishikant05 CreditAttribution: rishikant05 at Srijan | A Material+ Company commentedComment #32
rishikant05 CreditAttribution: rishikant05 at Srijan | A Material+ Company commentedSorry 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
Comment #34
prashantgoel CreditAttribution: prashantgoel as a volunteer commentedas mentioned in #25 this needs more work rather than just porting the patch from #24.
Thanks
Comment #35
no_angel CreditAttribution: no_angel as a volunteer commentedComment #36
no_angel CreditAttribution: no_angel as a volunteer commentedComment #37
Mile23Comment #38
Mile23Patch no longer applies.
Comment #39
naveenvalechaGiving 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.
Comment #40
Mile23Re: 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.
Comment #41
naveenvalechaThe data type should be "callable"
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 ?
Comment #42
jhodgdonYes please. If these are only API documentation blocks they should be 8.0.x.
Comment #43
naveenvalechaAddressing #41.1 and #41.2
phpcs reporting nothing.
Comment #44
Mile23Sorry, 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:
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 docblocksSo there are files which still have errors, including
system.admin.inc
.Comment #53
snehalgaikwad CreditAttribution: snehalgaikwad as a volunteer and at QED42 for Drupal India Association commentedAdding patch for both 8.9.x and 9.1.x versions.
Comment #54
abhisekmazumdarThanks @snehalgaikwad for the patches.
Both the paches looks good to me. RTBC from me.
Comment #55
quietone CreditAttribution: quietone as a volunteer commentedThis 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.