Problem/Motivation

Running php code sniffer shows errors and warnings.

Steps to reproduce

$ phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md .

Proposed resolution

Use phpcbf and manual effort to fix.

CommentFileSizeAuthor
#15 interdiff_9-15.txt587 bytessourabhjain
#15 3265732-15.patch8.65 KBsourabhjain
#9 interdiff_6-9.txt4.75 KBvictoria-marina
#9 Oracle_Eloqua_API_Redux-coding_standards_fix-3265732-9.patch8.64 KBvictoria-marina
#6 interdiff_4-6.txt482 bytesGuilherme Rabelo
#6 3265732-6.patch13.01 KBGuilherme Rabelo
#4 3265732-4.patch12.67 KBvictoria-marina
#4 interdiff_2-4.txt7.04 KBvictoria-marina
#2 3265732-2.patch7.67 KBsourabhjain

Comments

PapaGrande created an issue. See original summary.

sourabhjain’s picture

sourabhjain
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 3265732-2.patch7.67 KB

Resolved the coding standard issues. Please review it.

papagrande’s picture

papagrande
Primary language English
commented
Status: Needs review » Needs work

@sourabhjain, thank you for your help.

A few issues remain:

+++ b/src/Service/EloquaApiClient.php
@@ -185,11 +185,11 @@ class EloquaApiClient {
         // ksm(Json::decode($contents));

Please remove all commented out ksm() and other debugging code.

I'm also still seeing one phpcs warning:

FILE: ./src/Service/EloquaApiClient.php
----------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------------------------
142 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
----------------------------------------------------------------------------------------------

victoria-marina’s picture

victoria-marina commented
Status: Needs work » Needs review
StatusFileSize
new interdiff_2-4.txt7.04 KB
new 3265732-4.patch12.67 KB

Here is a patch. Kindly review it.

Guilherme Rabelo’s picture

Guilherme Rabelo commented
Assigned: papagrande » Guilherme Rabelo

I will review it.

Guilherme Rabelo’s picture

Guilherme Rabelo commented
Assigned: Guilherme Rabelo » Unassigned
StatusFileSize
new 3265732-6.patch13.01 KB
new interdiff_4-6.txt482 bytes

Only one issue appeared after applying the last patch.

FILE: /home/guilhermerabelo/Contribution/Project6/recommended-project/web/modules/contrib/eloqua_api_redux/src/Service/EloquaApiClient.php
--------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------------------------------------------------------------------
 360 | ERROR | unserialize() is insecure unless allowed classes are limited. Use a safe format like JSON or use the allowed_classes option.

So i just fix that.
Kindly review it.

papagrande’s picture

papagrande
Primary language English
commented
Status: Needs review » Needs work

@victoria-marina, @guilherme-rabelo, thank you for your help.

  • +++ b/eloqua_api_redux.services.yml
@@ -1,7 +1,7 @@
+    arguments: ['@config.factory', '@logger.factory', '@cache.default', '@http_client_factory', '@eloqua_api_redux.auth_fallback_default']

    This won't work because the eloqua_api_auth_fallback modules isn't always enabled. It's also not good to have circular dependencies between modules. 

  • +++ b/src/Service/EloquaApiClient.php
@@ -139,7 +150,7 @@ class EloquaApiClient {
-      $tokenGeneratorService = \Drupal::service('eloqua_api_redux.auth_fallback_default');

    Perhaps the best solution for now would be to ignore this warning by adding // phpcs:ignore DrupalPractice.Objects.GlobalDrupal.GlobalDrupal above the line.



    • Please also name your patches according to https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupal/patch-and-merge-request-guidelines#naming-conventions

    victoria-marina’s picture

    victoria-marina commented
    Assigned: Unassigned » victoria-marina
    victoria-marina’s picture

    victoria-marina commented
    Assigned: victoria-marina » Unassigned
    Status: Needs work » Needs review
    StatusFileSize
    new Oracle_Eloqua_API_Redux-coding_standards_fix-3265732-9.patch8.64 KB
    new interdiff_6-9.txt4.75 KB

    @PapaGrande thanks for your reply! Here is a patch following your considerations.

    tmaiochi’s picture

    tmaiochi commented
    Assigned: Unassigned » tmaiochi

    I'll do the review!

    tmaiochi’s picture

    tmaiochi commented
    Assigned: tmaiochi » Unassigned
    Status: Needs review » Reviewed & tested by the community

    The patch in comment #9 fixed all messages from PHPCS correctly, and following the comment #7. Moving to RTBC!

    papagrande’s picture

    papagrande
    Primary language English
    commented
    Status: Reviewed & tested by the community » Needs work

    @victoria-marina, @tmaiochi, thanks.

    +++ b/src/Service/Forms.php
@@ -153,8 +153,8 @@ class Forms {
+   *     supplied,1 will be used by default.

    One more small nit: add a space before "1".

    Have any of you tested the module with the patch applied?

    papagrande’s picture

    papagrande
    Primary language English
    commented
    Issue summary: View changes
    sourabhjain’s picture

    sourabhjain
    Primary language English
    commented
    Assigned: Unassigned » sourabhjain

    I will update this.

    sourabhjain’s picture

    sourabhjain
    Primary language English
    commented
    Assigned: sourabhjain » Unassigned
    Status: Needs work » Needs review
    StatusFileSize
    new 3265732-15.patch8.65 KB
    new interdiff_9-15.txt587 bytes

    Resolved the issue which is mentioned in #12
    Please review.

    papagrande’s picture

    papagrande
    Primary language English
    commented
    Status: Needs review » Fixed

    Thanks, all.

    Status: Fixed » Closed (fixed)

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