Problem/Motivation

There is a mistake in comment documenting function image of the Random class.

See: https://git.drupalcode.org/project/drupal/blob/8.8.x/core/lib/Drupal/Com...

  /**
   * Create a placeholder image.
   *
   * @param string $destination
   *   The absolute file path where the image should be stored.
   * @param int $min_resolution
   * @param int $max_resolution
   *
   * @return string
   *   Path to image file.
   */
  public function image($destination, $min_resolution, $max_resolution) {
    $extension = pathinfo($destination, PATHINFO_EXTENSION);
    $min = explode('x', $min_resolution);
    $max = explode('x', $max_resolution);

$min_resolution, $max_resolution are actually strings in the form of e.g. '400x400' , which becomes obvious after reading the beginning of the image method.

CommentFileSizeAuthor
#4 comment-fix-3076644-4.patch735 bytesLuke_Nuke
#2 comment-fix-3076644-2.patch659 bytesLuke_Nuke
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Luke_Nuke created an issue. See original summary.

Luke_Nuke’s picture

Luke_Nuke CreditAttribution: Luke_Nuke commented
Status: Active » Needs review
FileSize
comment-fix-3076644-2.patch659 bytes

Simple patch attached.

gueguerreiro’s picture

gueguerreiro
Primary language Portuguese, Portugal
Location Lisboa
CreditAttribution: gueguerreiro as a volunteer commented
Status: Needs review » Needs work

Nice catch, thank you for the patch.

Some nitpicks:

The following coding standards errors:

   * @param string $min_resolution
   *   E.g. '400x400'
268 | ERROR | [x] Parameter comment must end with a full stop
   * @param string $max_resolution
   *   E.g. '400x400'
270 | ERROR | [x] Parameter comment must end with a full stop

The example is fine, because the correct format the function expects is not immediately obvious. But I don't think they are a replacement for a description. I think it would be better if the parameters had a description, and then an example at the end. It can be something short and obvious, like "The minimum/maximum resolution for the image. E.g '400x400'.".

Luke_Nuke’s picture

Luke_Nuke CreditAttribution: Luke_Nuke commented
Status: Needs work » Needs review
FileSize
comment-fix-3076644-4.patch735 bytes

Thanks for a feedback. These are fixed now.

gueguerreiro’s picture

gueguerreiro
Primary language Portuguese, Portugal
Location Lisboa
CreditAttribution: gueguerreiro as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Great! That looks good, thank you.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Version: 8.8.x-dev » 8.7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed b7d22d5 and pushed to 8.8.x. Thanks!

diff --git a/core/lib/Drupal/Component/Utility/Random.php b/core/lib/Drupal/Component/Utility/Random.php
index 97df8cfc4a..3b50cce9a3 100644
--- a/core/lib/Drupal/Component/Utility/Random.php
+++ b/core/lib/Drupal/Component/Utility/Random.php
@@ -265,9 +265,9 @@ public function paragraphs($paragraph_count = 12) {
    * @param string $destination
    *   The absolute file path where the image should be stored.
    * @param string $min_resolution
-   *   The minimum resolution for the image. E.g '400x400'.
+   *   The minimum resolution for the image. For example, '400x300'.
    * @param string $max_resolution
-   *   The maximum resolution for the image. E.g '400x400'.
+   *   The maximum resolution for the image. For example, '800x600'.
    *
    * @return string
    *   Path to image file.

I think the g in e.g. is always supposed to be followed by a dot but rather than enter that discussion I chose to replace it with For example, as this is clearer and requires less latin :)

  • alexpott committed b7d22d5 on 8.8.x 
    Issue #3076644 by Luke_Nuke, gueguerreiro: Mistake in the comment...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Patch (to be ported) » Fixed

  • alexpott committed fb73b58 on 8.7.x 
    Issue #3076644 by Luke_Nuke, gueguerreiro: Mistake in the comment...

Status: Fixed » Closed (fixed)

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