[1.0.x] Google reviews

Thank you for applying! Reviewers will review the project files, describing what needs to be changed.

Please read Review process for security advisory coverage: What to expect → for more details and Security advisory coverage application checklist → to understand what reviewers look for. Tips for ensuring a smooth review gives some hints for a smoother review.

To reviewers: Please read How to review security advisory coverage applications → , What to cover in an application review → , and Drupal.org security advisory coverage application workflow → .

While this application is open, only the user who opened the application can make commits to the project used for the application.

Reviewers only describe what needs to be changed; they don't provide patches to fix what reported in a review.

It seems you have missed working on the coding standards. You can use the PHPCS tool for checking and resolving issues.

phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml googlereviews/

FILE: googlereviews/googlereviews.module
-------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
-------------------------------------------------------------------------------------
 18 | WARNING | [x] A comma should follow the last multiline array item. Found: NULL
 24 | WARNING | [x] A comma should follow the last multiline array item. Found: NULL
-------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------------


FILE: googlereviews/README.md
----------------------------------------------------------------------
FOUND 0 ERRORS AND 8 WARNINGS AFFECTING 8 LINES
----------------------------------------------------------------------
  2 | WARNING | Line exceeds 80 characters; contains 82 characters
  5 | WARNING | Line exceeds 80 characters; contains 147 characters
  8 | WARNING | Line exceeds 80 characters; contains 275 characters
 11 | WARNING | Line exceeds 80 characters; contains 90 characters
 15 | WARNING | Line exceeds 80 characters; contains 85 characters
 16 | WARNING | Line exceeds 80 characters; contains 201 characters
 18 | WARNING | Line exceeds 80 characters; contains 227 characters
 20 | WARNING | Line exceeds 80 characters; contains 176 characters
----------------------------------------------------------------------


FILE: googlereviews/src/Form/SettingsForm.php
---------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AND 3 WARNINGS AFFECTING 2 LINES
---------------------------------------------------------------------------------------------------------------------------------------------
 35 | WARNING | [ ] t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 35 | ERROR   | [x] Expected one space after the comma, 0 found
 35 | WARNING | [x] A comma should follow the last multiline array item. Found: )
 41 | WARNING | [ ] t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
---------------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------------------------------------------------------------------------


FILE: googlereviews/src/GetGoogleData.php
----------------------------------------------------------------------------------------------------------------------
FOUND 17 ERRORS AND 1 WARNING AFFECTING 13 LINES
----------------------------------------------------------------------------------------------------------------------
  46 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
  47 | ERROR   | [x] Expected 5 space(s) before asterisk; 3 found
  48 | ERROR   | [x] Expected 5 space(s) before asterisk; 3 found
  49 | ERROR   | [x] Expected 5 space(s) before asterisk; 3 found
  50 | ERROR   | [x] Expected 5 space(s) before asterisk; 3 found
  67 | ERROR   | [ ] Parameter $config_factory is not described in comment
  76 | ERROR   | [ ] Doc comment for parameter $configFactory does not match actual variable name $string_translation
  97 | ERROR   | [x] There must be exactly one blank line before the tags in a doc comment
 108 | ERROR   | [x] Expected 1 space after IF keyword; 0 found
 114 | ERROR   | [x] Expected 1 space after IF keyword; 0 found
 118 | ERROR   | [x] Expected 1 space after IF keyword; 0 found
 122 | ERROR   | [x] Concat operator must be surrounded by a single space
 122 | ERROR   | [x] Concat operator must be surrounded by a single space
 122 | ERROR   | [x] Concat operator must be surrounded by a single space
 122 | ERROR   | [x] Concat operator must be surrounded by a single space
 122 | ERROR   | [x] Concat operator must be surrounded by a single space
 122 | ERROR   | [x] Concat operator must be surrounded by a single space
 126 | WARNING | [ ] Unused variable $status.
----------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 15 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------------------------------------------------------


FILE: googlereviews/src/Plugin/Block/GoogleRatingBlock.php
-------------------------------------------------------------------------------------------------------------------------------------------
FOUND 20 ERRORS AND 1 WARNING AFFECTING 16 LINES
-------------------------------------------------------------------------------------------------------------------------------------------
  8 | ERROR   | [x] When importing a class with "use", do not include a leading \
 29 | ERROR   | [ ] Missing short description in doc comment
 29 | ERROR   | [ ] Parameter $getGoogleData is not described in comment
 30 | ERROR   | [ ] Missing parameter comment
 31 | ERROR   | [ ] Missing parameter comment
 32 | ERROR   | [ ] Missing parameter comment
 34 | ERROR   | [x] Expected 1 blank line before function; 2 found
 39 | ERROR   | [ ] Missing short description in doc comment
 40 | ERROR   | [ ] Missing parameter comment
 41 | ERROR   | [ ] Missing parameter comment
 42 | ERROR   | [ ] Missing parameter comment
 43 | ERROR   | [ ] Missing parameter comment
 60 | ERROR   | [ ] The array declaration extends to column 90 (the limit is 80). The array content should be split up over multiple lines
 60 | ERROR   | [x] Short array syntax must be used to define arrays
 62 | ERROR   | [x] Expected 1 space after IF keyword; 0 found
 64 | ERROR   | [x] Expected 1 space before "=>"; 0 found
 68 | ERROR   | [x] Expected 1 space before "/"; 0 found
 68 | ERROR   | [x] Expected 1 space after "/"; 0 found
 68 | ERROR   | [x] Expected 1 space before "*"; 0 found
 68 | ERROR   | [x] Expected 1 space after "*"; 0 found
 69 | WARNING | [x] A comma should follow the last multiline array item. Found: ]
-------------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 10 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------------------------------------------------------------------


FILE: googlereviews/src/Plugin/Block/GoogleReviewsBlock.php
-------------------------------------------------------------------------------------------
FOUND 14 ERRORS AND 1 WARNING AFFECTING 14 LINES
-------------------------------------------------------------------------------------------
  8 | ERROR   | [x] When importing a class with "use", do not include a leading \
 28 | ERROR   | [ ] Missing short description in doc comment
 28 | ERROR   | [ ] Parameter $getGoogleData is not described in comment
 29 | ERROR   | [ ] Missing parameter comment
 30 | ERROR   | [ ] Missing parameter comment
 31 | ERROR   | [ ] Missing parameter comment
 38 | ERROR   | [ ] Missing short description in doc comment
 39 | ERROR   | [ ] Missing parameter comment
 40 | ERROR   | [ ] Missing parameter comment
 41 | ERROR   | [ ] Missing parameter comment
 42 | ERROR   | [ ] Missing parameter comment
 59 | ERROR   | [x] Short array syntax must be used to define arrays
 61 | ERROR   | [x] Expected 1 space after IF keyword; 0 found
 63 | ERROR   | [x] Expected 1 space before "=>"; 0 found
 66 | WARNING | [x] A comma should follow the last multiline array item. Found: ]
-------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 5 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------------------

Thanks for the feedback. I committed the recommended changes to the repository.

Hello @ptitb,

I have reviewed the changes, and they look fine to me.

Let’s wait for other reviewers to take a look and if everything goes fine, you will get the role.

Thanks

• The following points are just a start and don't necessarily encompass all of the changes that may be necessary
• A specific point may just be an example and may apply in other places
• A review is about code that doesn't follow the coding standards, contains possible security issue, or doesn't correctly use the Drupal API; the single points aren't ordered, not even by importance

src/GetGoogleData.php

    if ($auth_key == '' || $place_id == '') {
      $link = $this->urlGenerator->generateFromRoute('googlereviews.settings_form');
      $this->messenger->addError($this->stringTranslation->translate('You need to add credentials on the <a href=":link">Google review setings page</a> to show the reviews.', [':link' => $link]));
      return;
    }

$this->stringTranslation->translate() should not be used to translate strings, as its documentation says.

Never call this translate() method directly. In order for strings to be localized, make them available in one of the ways supported by the Localization API. When possible, use the \Drupal\Core\StringTranslation\StringTranslationTrait $this->t(). Otherwise create a new \Drupal\Core\StringTranslation\TranslatableMarkup object.

    catch (RequestException $e) {
      $this->logger->get('googlereviews')->error($e->getMessage());
    }

For exceptions, the function used to log them should be watchdog_exception(), or code similar to that should be used.

googlereviews.info.yml

package: Custom

Modules should avoid to use that value for package.

Thanks @apaderno! Took your recommendations and fixed and committed them.
I now see I was a bit too early with replacing watchdog_exception().

I started development on some new features in the module and would like to create a 1.1.0 branch and tag. Do you want me to change the title of this issue to reflect this new branch?

We already started reviewing this branch. You can create a new branch, but we will continue with this.

Okay, thanks!

Hi @ptib

Nice module and good job!

I did a smoke review and there are already some mentioned things above. What can I add is:

• The method getGoogleReviews() should return the type :array
• I am not sure that you should keep the $api_url = 'https://maps.googleapis.com/maps/api/place/details/json'; inside the getGoogleReviews() method. From how I see it, it's better to keep it as a config, so, you'll be able to change it when is necessary.
• The permission administer googlereviews configuration was used in the googlereviews.routing.yml file but there is no such defined permission in your module. You should probably add the googlereviews.permissions.yml file and define it.

Thanks @andrei.vesterli! Good points.
Fixed and committed the changes.

@ptitb

Nice job!

For me, all is fine now. Let's see what others will say.

Regards,
Andrei

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