Needs cleanup adhering to Drupal coding standard.

CommentFileSizeAuthor
#16 coding_standards-2837985-16.patch9.63 KBgmaltoni
#14 12.PNG17.07 KBbhavikshah9
#12 logintoboggan_reminder-coding-standard-2837985-12-d7.patch6.54 KBlomasr
#11 after.png85.41 KBlomasr
#10 interdiff-2837985-5-10.txt2.83 KBbhavikshah9
#10 logintoboggan_reminder-coding-standard-2837985-10-d7.patch6.85 KBbhavikshah9
#5 interdiff-2837985-2-5.txt1.03 KBbhavikshah9
#5 logintoboggan_reminder-coding-standard-2837985-5-d7.patch6.59 KBbhavikshah9
#2 logintoboggan_reminder-coding-standard-2837985-2-d7.patch6.61 KBbhavikshah9

Comments

bhavikshah9 created an issue. See original summary.

bhavikshah9’s picture

bhavikshah9 commented
Status: Active » Needs review
StatusFileSize
new logintoboggan_reminder-coding-standard-2837985-2-d7.patch6.61 KB

Here is the patch.

richardp’s picture

richardp commented

Thank you-- This looks good to me, with one change:

I wanted to intentionally allow markup in the reminder msg that gets displayed in hook_init(), just in case some site admins want to be fancy with the message, display links to other pages, etc. Using check_plain() on it would strip any such HTML.

In the meantime, I'm afraid I don't have much time these days to tend to this project. Would you be interested in being made a co-maintainer?

Richard

bhavikshah9’s picture

bhavikshah9 commented

Hi @richardp,

Thanks for prompt reply.
I have used check_plain() as a security aspect. But, your argument is valid. I will replace check_plain() with filter_xss_admin().

And yes, I would be glad to become co-maintainer.

bhavikshah9’s picture

bhavikshah9 commented
StatusFileSize
new logintoboggan_reminder-coding-standard-2837985-5-d7.patch6.59 KB
new interdiff-2837985-2-5.txt1.03 KB

Here is the patch.

richardp’s picture

richardp commented

Looks good to me. Please feel free to apply the patch and commit your changes whenever you like.
Richard

bhavikshah9’s picture

bhavikshah9 commented

Hey @richardp,

I will keep it open as it for a week for community members to look into it.
Then, commit it.

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
commented 
  1. +++ b/logintoboggan_reminder.module
@@ -3,126 +3,107 @@
+    "#default_value" => variable_get("logintoboggan_reminder_msg", check_plain($default_msg)), ¶

    extra white space 

  2. +++ b/logintoboggan_reminder.module
@@ -3,126 +3,107 @@
+    "#options" => array(
+      "status" => "status",
+      "warning" => "warning",
+      "error" => "error"

    shouldn't this be also wrapped in t() as its for UI ?

bhavikshah9’s picture

bhavikshah9 commented

Hey @joshi.rohit100,

Thanks for the review. I don't know why my coder doesn't sniffed this.
I will update the new patch and interdiff soon.

bhavikshah9’s picture

bhavikshah9 commented
StatusFileSize
new logintoboggan_reminder-coding-standard-2837985-10-d7.patch6.85 KB
new interdiff-2837985-5-10.txt2.83 KB

Here is the updated patch with interdiff.

lomasr’s picture

lomasr commented
StatusFileSize
new after.png85.41 KB

Applied the patch , seems to have 'whitespace issues' . Please see the screenshot.

lomasr’s picture

lomasr commented
StatusFileSize
new logintoboggan_reminder-coding-standard-2837985-12-d7.patch6.54 KB

I have tried to remove whitespaces. Please review the patch.

bhavikshah9’s picture

bhavikshah9 commented

Hey @lomasr,

Thanks for the screenshot.
Yes. You are right about the warnings. I have got similar warnings while development. But, I didn't found any whitespace actually in code.
Have you found any whitespace actually within module file after apply the patch?
Or, can you please help me understand why it's showing warnings when there are no actual whitespace?

bhavikshah9’s picture

bhavikshah9 commented
StatusFileSize
new 12.PNG17.07 KB

Hey @lomasr,

I have tried to apply your patch.
Unfortunately, it shows whitespace messages as well. In fact, patch failed to be applied. I have attached a screenshot.

I have no idea why such warnings comes up. This is something I have never faced.
I will keep this issue open for few more days for community members to comment/review.

lomasr’s picture

lomasr commented

Hi , I like to maintain this module

gmaltoni’s picture

gmaltoni commented
Issue tags: +#ciandt-contrib
StatusFileSize
new coding_standards-2837985-16.patch9.63 KB

Hi!

Just a few coding standards improvements.

I hope to help.

Thanks.

bhavikshah9’s picture

bhavikshah9 commented

Hey @lomasr,

Thanks. But, this module is not actively maintained.

faline’s picture

faline commented
Status: Needs review » Reviewed & tested by the community

I've applied the patch #16 and it is working for me, with no warning.

lomasr’s picture

lomasr commented

Hi, thanks. In this case who is going to commit and close this issue? , Not sure.

richardp’s picture

richardp commented

lomsar, I have added you as a maintainer. Please feel free to commit and close the issue as you see fit.

Thanks,
Richard

lomasr’s picture

lomasr commented

Thanks Richard.

  • lomasr committed 6b52543 on 7.x-1.x authored by gmaltoni 
    Issue #2837985 by bhavikshah9, lomasr, gmaltoni, richardp, joshi....
lomasr’s picture

lomasr commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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