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 Block 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 (20-25 as a guess).

How To Review This Issue

  1. Attempt to apply the patch to see if it needs a reroll.
  2. Use the phpcs one-liner to evaluate whether all the relevant standards errors have been resolved: https://gist.github.com/paul-m/227822ac7723b0e90647
  3. Look at each change and determine whether the type hint is correct.

Related sprint issues:

Sprint Topic Sub Issue
#1518116: [meta] Make Core pass Coder Review #1522384: Make Block module pass Coder Review
#1310084: [meta] API documentation cleanup sprint #1807652: Further clean up of API docs for Block module
#500866: [META] remove t() from assert message #1741338: Removing t() from asserts in simpletests in block module
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lars Toomre’s picture

Status: Active » Needs review
FileSize
7.03 KB

Here is a patch that addresses missing type hinting in the block.api.php file.

Lars Toomre’s picture

Status: Needs review » Postponed
mgifford’s picture

Issue summary: View changes
Status: Postponed » Closed (won't fix)

unless there is another way around this.

Mile23’s picture

Status: Closed (won't fix) » Needs review
FileSize
1.81 KB

Fixes all parameter hinting errors in docblocks that aren't in block.api.php.

In fact, block.api.php doesn't have any such errors at this point.

Mile23’s picture

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

Applies nicely. Makes sense to me to add this inline documentation.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs review

Umm...

+ * @param string $theme
  *   The theme to rehash blocks for. If not provided, defaults to the currently
  *   used theme.

Is this optional? The docs look like it is, but it doesn't say (optional), and what is the default, still a string or NULL?

 * @param array $theme_list
  *   An array of theme names.

should be string[]?

Mile23’s picture

FileSize
1.95 KB
924 bytes

Thanks for the review, @jhodgdon.

Fixed both things.

mgifford’s picture

Nice catch. Anyone want to mark this RTBC? Seems like a fairly straight forward set of improvements to the inline docs.

Mile23’s picture

@mgifford: You could mark it RTBC, if it still applies and is a good patch.

mgifford’s picture

It still applies. It also seems to conform with other examples, but this isn't an area I'm confident about.

The closest documentation I could find on this was:
https://www.drupal.org/node/1354#functions
https://www.drupal.org/node/608152#hinting

Can you point me better documentation? Just realizing that to me this looks like an array string[] made me realize I've got to know more before marking it RTBC.

mgifford queued 8: 1807674_8.patch for re-testing.

Mile23’s picture

Still applies.

Mile23’s picture

Still applies.

Mile23’s picture

Issue summary: View changes
deepakaryan1988’s picture

Re-rolling the patch of @Mile23 as there was few minor thing were there.
:)

yogen.prasad’s picture

Status: Needs review » Reviewed & tested by the community

Looking fine

xjm’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/block/block.module
@@ -126,7 +126,7 @@ function _block_rehash($theme = NULL) {
- * @param $theme_list
+ * @param string $theme_list
  *   An array of theme names.

If it's an array, it's not a string. Perhaps this should be string[]? Can we check what the function expects by looking over its code?

deepakaryan1988’s picture

Status: Needs work » Needs review
FileSize
1.95 KB
372 bytes

@xjm Agreed!!
Ahh I missed it. Thanks for pointing out.
Re-rolling the patch

Mile23’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs review » Needs work
Issue tags: +Needs reroll

Bumping to 8.1.x, no longer applies.

naveenvalecha’s picture

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

Admins-MacBook-Pro% phpcs --standard="Drupal" --extensions="module/php,php" --report-csv core/modules/block | grep -F $'Missing param\nReturn type missing'

madhavvyas’s picture

Changes looks good to me.

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

Nice, thanks.

phpcs reports no missing @param or @returns hints. All the hints look correct.

Also: Easy review. :-)

jhodgdon’s picture

Version: 8.1.x-dev » 8.0.x-dev
Status: Reviewed & tested by the community » Needs work

There is no reason this patch, if corrected, could not be committed to 8.0.x.

However, I do not think it is quite correct:

  1. +++ b/core/modules/block/src/BlockViewBuilder.php
    @@ -182,9 +182,9 @@ protected static function buildPreRenderableBlock($entity, ModuleHandlerInterfac
    +   * @param int $entity_id
        *   A block config entity ID.
    

    I seriously doubt that this is an integer. Most config entity IDs are strings. Please check this again.

  2. +++ b/core/modules/block/src/BlockViewBuilder.php
    @@ -182,9 +182,9 @@ protected static function buildPreRenderableBlock($entity, ModuleHandlerInterfac
    -   * @param $view_mode
    +   * @param string $view_mode
        *   The view mode the block is being viewed in.
    

    Is this really a string or an object? Please verify.

naveenvalecha’s picture

Re:#24.1

  public static function lazyBuilder($entity_id, $view_mode) {
    return static::buildPreRenderableBlock(entity_load('block', $entity_id), \Drupal::service('module_handler'));
  }

I have checked the type of entity_id in entity_load and its mixed.


Re : #24.2
I have confirmed view_mode is a string.check EntityViewBuilderInterface viewMultiple function.

So probably we need a reroll for #24.1

jhodgdon’s picture

RE #25 ...

Yes, in entity_load(), entity IDs are allowed to be mixed. However, in this class it is specifically a block entity, so its ID is a string. If it were a node or comment, its ID would be an integer, but this is a block config entity so it is a string.

Agreed on the view mode though that it does seem to be expecting a string like 'full' for this.

naveenvalecha’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
586 bytes
586 bytes

#26.1
Agreed, the id is string Drupal\block\Entity\Block
changed the id data type to string.
Changing the status to RTBC as per #23 b/c the above change has been addressed.
phpcs reports no missing @param or @returns hints.

naveenvalecha’s picture

FileSize
1.35 KB

The last submitted patch, 27: 1807674-27.patch, failed testing.

jhodgdon’s picture

Thanks! The changes that are in the patch, I believe, are correct now.

I have not marked this patch RTBC however... Please see instructions in the issue summary. Someone (not @naveenvalecha) needs to do step 2 under "How to review this issue" to verify that the Block module has been completely patched with @param/@return types before we can call this issue done. Thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 28: 1807674-27.patch, failed testing.

karolus’s picture

I tested by following the steps for review, and ran automatic fixes via PHPCS into patch #28.

Mile23’s picture

Status: Needs work » Needs review

Setting 'needs review.'

jhodgdon’s picture

Status: Needs review » Needs work

Sorry, but the patch in #32 has some problems and should not be committed, and includes some out-of-scope fixes. Please discard.

So. Once again: Please someone go back to the patch in #28 and verify that step #2 under "How to review" has passed. And upload the patch in #28 again.

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.

shetpooja04’s picture

Status: Needs work » Needs review
FileSize
1.32 KB

I have uploaded patch with changes made in #28 in 8.9.x version
as suggested in #34

used the below command to check https://gist.github.com/paul-m/227822ac7723b0e90647

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

Please review !

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.