There are some minor errors in in the coding standards.

CommentFileSizeAuthor
#7 interdif.txt4.14 KBPavan B S
#7 2868508-7.patch1.91 KBPavan B S
#3 Before_mass-contact1.png27.18 KBriddhi.addweb
#3 Before_mass-contact.png209.56 KBriddhi.addweb
#3 After_mass-contact.png63.32 KBriddhi.addweb
#2 2868508-2.patch6.5 KBPavan B S
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Pavan B S created an issue. See original summary.

Pavan B S’s picture

Pavan B S CreditAttribution: Pavan B S at Valuebound commented
Assigned: Pavan B S » Unassigned
Status: Active » Needs review
FileSize
2868508-2.patch6.5 KB

Applying the patch, please review.

riddhi.addweb’s picture

riddhi.addweb CreditAttribution: riddhi.addweb at AddWeb Solution Pvt. Ltd. commented
FileSize
After_mass-contact.png63.32 KB
Before_mass-contact.png209.56 KB
Before_mass-contact1.png27.18 KB

Thanks @Pavan for patch, I have checked patch on simplytest.me with the help of coder module and it solves all critical and normal warnings. PFA Screenshots.

riddhi.addweb’s picture

riddhi.addweb CreditAttribution: riddhi.addweb at AddWeb Solution Pvt. Ltd. commented
Status: Needs review » Reviewed & tested by the community
oadaeh’s picture

oadaeh CreditAttribution: oadaeh at Hook 42 commented
Status: Reviewed & tested by the community » Needs work

Thank you for submitting the patch. However, some of your changes are not correct.

In the last 7 hunks, you actually make the module worse by removing the calls to t() and replacing them with check_plain(), because now none of that text can be translated on the site. All of those !placeholders that you were warned about were created in code and have no user input, so there is no reason to run them through check_plain().

If you will notice the text of the error, it says they are "potential" problems. Understanding the code and what is happening and what can happen is important. Blindly following a machine and making changes without understanding them is not a good idea.

I will not accept or commit the patch as submitted.

Pavan B S’s picture

Pavan B S CreditAttribution: Pavan B S at Valuebound commented

@oadeh sorry for the mistake in the patch, i was not knowing about that issue, i will revert the patch, by removing the check_plain()

Pavan B S’s picture

Pavan B S CreditAttribution: Pavan B S at Valuebound commented
Status: Needs work » Needs review
FileSize
2868508-7.patch1.91 KB
interdif.txt4.14 KB

Re applying the patch, please review

  • oadaeh committed a0e0154 on 7.x-1.x authored by Pavan B S 
    Issue #2868508 by Pavan B S, Jigar.addweb, oadaeh: Fix coding standards
oadaeh’s picture

oadaeh CreditAttribution: oadaeh at Hook 42 commented
Status: Needs review » Fixed

Thanks for updating the patch. It is committed, so I'm marking this as fixed.

Status: Fixed » Closed (fixed)

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