Found few functions has unnecessary @param tags

  1. File : core/modules/block/src/BlockViewBuilder.php
    Function : lazyBuilder()
  2. File : core/modules/block/src/Tests/Migrate/d6/MigrateBlockTest.php
    Function : assertEntity()

Few function missing variable name for @param tags.

    File : core/modules/block_content/src/Plugin/Block/BlockContentBlock.php
    Function : __construct()
CommentFileSizeAuthor
#11 interdiff-2601682-9-11.txt1.01 KBkrknth
#11 2601682-8.patch4.14 KBkrknth
#9 2601682-7.patch4.14 KBkrknth
#6 interdiff-2601682-5-6.txt883 bytesrakesh.gectcr
#6 interdiff-2601682-2-6.txt856 bytesrakesh.gectcr
#6 2601682-6.patch3.82 KBrakesh.gectcr
#5 interdiff-2601682-2-5.txt950 bytesanil280988
#5 2601682-5.patch3.8 KBanil280988
#3 2601682-2.patch3.77 KBkrknth
#2 2601682-1.patch1.13 KBkrknth
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

krknth created an issue. See original summary.

krknth’s picture

krknth CreditAttribution: krknth as a volunteer and at Valuebound commented
Assigned: krknth » Unassigned
Status: Active » Needs review
FileSize
2601682-1.patch1.13 KB

Added patch

krknth’s picture

krknth CreditAttribution: krknth as a volunteer and at Valuebound commented
Title: Remove unnecessary @param tags in block module » Unnecessary @param tags & Missing variable names in block, block_content module
Issue summary: View changes
FileSize
2601682-2.patch3.77 KB

Addded new patch that will fix in block_content module too.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

This all looks good, thanks!

One thing though:

+++ b/core/modules/block_content/src/Plugin/Block/BlockContentBlock.php
@@ -66,12 +66,13 @@ class BlockContentBlock extends BlockBase implements ContainerFactoryPluginInter
+   * @param \Drupal\Core\Routing\UrlGeneratorInterface $url_generator

If you are going to add a @param tag line, it also needs a documentation line below it.

anil280988’s picture

anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commented
Status: Needs work » Needs review
FileSize
2601682-5.patch3.8 KB
interdiff-2601682-2-5.txt950 bytes

Added Documentation line.

rakesh.gectcr’s picture

rakesh.gectcr
he/him
Primary language Malayalam
Location Manchester
CreditAttribution: rakesh.gectcr as a volunteer and at Valuebound commented
FileSize
2601682-6.patch3.82 KB
interdiff-2601682-2-6.txt856 bytes
interdiff-2601682-5-6.txt883 bytes
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community

Patch #5 is correct. Patch #6 isn't -- the parameter is actually a class that generates URLs, not a generated URL and there is no given route anyway.

So, #5 is RTBC. Hiding #6 patch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/block_content/src/Plugin/Block/BlockContentBlock.php
@@ -66,12 +66,14 @@ class BlockContentBlock extends BlockBase implements ContainerFactoryPluginInter
+   * @param \Drupal\Core\Routing\UrlGeneratorInterface $url_generator
+   *   The url generator.

This class is also missing a protected property for $this->urlGenerator - let's add that here.

krknth’s picture

krknth CreditAttribution: krknth as a volunteer and at Valuebound commented
Status: Needs work » Needs review
FileSize
2601682-7.patch4.14 KB

@alexpott : Added protected $urlGenerator

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

An interdiff would have been nice. ;)

Sorry, didn't notice this either in my previous reviews:

  1. +++ b/core/modules/block_content/src/Plugin/Block/BlockContentBlock.php
@@ -58,6 +58,13 @@ class BlockContentBlock extends BlockBase implements ContainerFactoryPluginInter
+   * The url generator.

    url => URL 

  2. +++ b/core/modules/block_content/src/Plugin/Block/BlockContentBlock.php
@@ -66,12 +73,14 @@ class BlockContentBlock extends BlockBase implements ContainerFactoryPluginInter
+   *   The url generator.

    url => URL

krknth’s picture

krknth CreditAttribution: krknth as a volunteer and at Valuebound commented
Status: Needs work » Needs review
FileSize
2601682-8.patch4.14 KB
interdiff-2601682-9-11.txt1.01 KB

@jhodgdon : sorry for not adding interdiff before
fixed your comment

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community

Thanks!

The last submitted patch, 5: 2601682-5.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed 3060a7c and pushed to 8.0.x. Thanks!

+++ b/core/modules/block_content/src/Plugin/Block/BlockContentBlock.php
@@ -58,6 +58,13 @@ class BlockContentBlock extends BlockBase implements ContainerFactoryPluginInter
   /**
+   * The URL generator.
+   *
+   * @var \Drupal\Core\Routing\UrlGeneratorInterface
+   */
+  protected $urlGenerator;

Technically this is an API change - but I think it is totally acceptable to protect this. Nothing should be publicly accessing this.

  • alexpott committed 3060a7c on 
    Issue #2601682 by krknth, rakesh.gectcr, anil280988, jhodgdon:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.