Code Review

After going through the pareview.sh errors. I found out the Drupal dependencies errors. I solved all of them and made a patch. Please review and mention if any more changes are to be made. I'll be sending patches regarding the other errors too as soon as possible. Thank you.

Many thanks bhanuprakashnani! I'll go through your patch asap.

This patch takes care of the errors regarding changing the syntax from t() to $this->t(). Please review and mention if any more changes are to be made. Thank you.

This patch consists of the short array syntaxes. I have changed all the syntaxes of the array to short array. Please review and mention if any more changes are to be made. Thank you.

This patch includes the correction of line break of 80 characters in the README.d file. Please review and mention if any more changes are to be made. Thank you.

Please let me know what else is to be changed. I am interested in helping through this errors. Thank you.

This patch takes care of the @file tag doc blocks. I have removed all of them as per their Drupal standards. Please review and mention if any more changes are to be made. Thank you.

Bhanuprakashnani,

Apologies for the delay in responding. I was totally smashed by work recently. I'm checking through your patches now. Many thanks!

Hi Bhanuprakashnani,

I have an update to #5: there was an additional drupal_message() that was using a string literal, without the translate function. I have not addressed the "Only string literals should be passed to t() where possible" warnings, because the translate config items. These are not really a problem, IMO.

Could you please review the new patch.

Hi Bhanuprakashnani,

I am unable to apply patches:

• 2969236-7.patch
• 2969236-8.patch

error: patch failed: README.md:47
error: README.md: patch does not apply

In addition, your readme patches overrode the title levels to all h1. Please maintain the title hierarchy (unless I have made mistakes)

The following patches have been applied:

• 2969236-3.patch
• 2969236-4.patch
• 2969236-6.patch

Many thanks

I did not get any commit credit for the commit messages above. Will I be getting once the issue is fixed or should I be getting it now itself. Please let me know. Thank you.

hi Bhanuprakashnani,

I credited you in both commits:

• 67ad929cbd9f20cf0f74147ab9495cbb5e06dc52 : "Issue #2969236 by bhanuprakashnani: fix Drupal dependencies errors"
• a6a2d7d04253e4a5bc42fedca69a89eba331e153 : "Issue #2969236 by bhanuprakashnani: fix pareview short array issues"

What was ommitted?

Yeah I saw that u committed it. But my doubt was shouldnt i be getting the name of this module under my commits? I dont see it in my commit list.

Hi Bhanuprakashnani,

I think that can only happen if you perform commit from your machine, as your user. Because I did the merge and commit from my machine, the commit is shown as done by me. see https://www.drupal.org/node/711070:

It is customary in the commit message to reference the node ID of the issue in your project's issue queue where the bug/feature request was raised, and to mention any contributors who helped with the code: "Issue #[issue number] by [comma-separated usernames]: [Short summary of the change]."

You would type the full commands as follows:

git add -u
git commit -m "Issue #[issue number] by [comma-separated usernames]: [Short summary of the change]."

I am happy to add you as a co-maintainer, if you want to perform your own commits,

Thank you so much. I would be very happy to be a co-maintainer and contribute to this module. Please give me the permissions for it. I ll do my best in developing the module through my contributions.

And I will send the other patches again correcting the errors. Thank you for the support.

No problem, you've been added as a VCS contributor. I've updated the main branch to 8.x-1.x (it was the rc1 branch, which was incorrect). This is probably what caused you patches to fail before (I think your patches were from the rc1 branch).
.

Thank you so much. Tell me how can I be of help to develop this module. Will always step forward to learn new things and work for it.

Made the patch again. Please check it.

If I want to commit it. What is the process? Can u please let me know?

Hi Bhanu,

I couldn't apply the patch again. I'm jot sure why this is. Doing a verbose apply showed that the issue was at line 37 - the first set of indentations. Could you make sure that you do not have tab characters enables in your editor? I doubt that's the cause, but it's the only thing I can think of at the moment.

I've made a commit to 8.1-1.x and merged those changes into your dev branch for this ticket. Please make sure you do a git pull. I've also made a patch myself, can you see if you can apply it to a checkout of your branch? If that fails, there must be something else going on. Assuming it works, please review it and make any changes you see need making. Also, please leave the titles with the '#' style not '-', this is a better standard because it allows for > 2 sub-title levels.

Re your questions about committing, Git workflow:

Please make sure that you read https://www.drupal.org/node/711070, https://www.drupal.org/node/52287.

I have made a few changes to the project, so that we follow drupal.org standards more closely.

8.1-1.x is our master branch.

8.1-1.x-dev is out dev branch.

You should create feature branches from the dev branch for each ticket.

When we're happy with the changes in the feature, you can merge your feature branch into dev and then delete your feature branch from the remote.

I will merge dev into master and make a tag & release when we are ready for the next release.

If you need any help or advice with git and git workflow, just ask, no problem.

Thank you so much. Very happy to work with you. I ll ask you whenever I have some queries.

8.x-1.x-dev now has the following issues:

FILE: .../custom/marketing_cloud/modules/marketing_cloud_assets/README.md
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 4 | ERROR | [x] Expected 1 newline at end of file; 0 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...eting_cloud/modules/marketing_cloud_assets/src/AssetsService.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 84 | WARNING | Line exceeds 80 characters; contains 186 characters
----------------------------------------------------------------------


FILE: ...marketing_cloud/modules/marketing_cloud_push/src/PushService.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
----------------------------------------------------------------------
  23 | WARNING | Line exceeds 80 characters; contains 148 characters
  97 | WARNING | Line exceeds 80 characters; contains 148 characters
 358 | WARNING | Line exceeds 80 characters; contains 182 characters
----------------------------------------------------------------------


FILE: ...g_cloud/modules/marketing_cloud_contacts/src/ContactsService.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 12 WARNINGS AFFECTING 12 LINES
----------------------------------------------------------------------
  23 | WARNING | Line exceeds 80 characters; contains 91 characters
  36 | WARNING | Line exceeds 80 characters; contains 92 characters
  52 | WARNING | Line exceeds 80 characters; contains 87 characters
  68 | WARNING | Line exceeds 80 characters; contains 84 characters
 100 | WARNING | Line exceeds 80 characters; contains 113 characters
 148 | WARNING | Line exceeds 80 characters; contains 116 characters
 170 | WARNING | Line exceeds 80 characters; contains 116 characters
 208 | WARNING | Line exceeds 80 characters; contains 112 characters
 225 | WARNING | Line exceeds 80 characters; contains 84 characters
 241 | WARNING | Line exceeds 80 characters; contains 94 characters
 257 | WARNING | Line exceeds 80 characters; contains 113 characters
 300 | WARNING | Line exceeds 80 characters; contains 142 characters
----------------------------------------------------------------------


FILE: ...d/modules/marketing_cloud_interaction/src/InteractionService.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 10 WARNINGS AFFECTING 10 LINES
----------------------------------------------------------------------
  23 | WARNING | Line exceeds 80 characters; contains 173 characters
  36 | WARNING | Line exceeds 80 characters; contains 493 characters
  52 | WARNING | Line exceeds 80 characters; contains 311 characters
  97 | WARNING | Line exceeds 80 characters; contains 126 characters
 113 | WARNING | Line exceeds 80 characters; contains 462 characters
 145 | WARNING | Line exceeds 80 characters; contains 179 characters
 165 | WARNING | Line exceeds 80 characters; contains 140 characters
 190 | WARNING | Line exceeds 80 characters; contains 199 characters
 240 | WARNING | Line exceeds 80 characters; contains 124 characters
 259 | WARNING | Line exceeds 80 characters; contains 143 characters
----------------------------------------------------------------------


FILE: ...m/marketing_cloud/modules/marketing_cloud_sms/src/SMSService.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
  55 | WARNING | Line exceeds 80 characters; contains 92 characters
 274 | WARNING | Line exceeds 80 characters; contains 191 characters
----------------------------------------------------------------------

The remaining issue are the service classes, where the short description is lifted directly from the Salesforce documentation, to prevent confusion. IMO, an acceptable breach of warnings.

The other issue (marketing_cloud_assets/README.md: Expected 1 newline at end of file; 0 found) will be resolved in #2974148 (README.md files required for all base and sub modules).

Closing this ticket, because reported issues have been resolved, apart from dependency injection warnings (I feel this is a false negative), and lines > 80 chars (these are short descriptions, as supplied by SF, and should be kept the same to prevent any confusion).

Any further code sniffer warnings can be assigned their own tickets,