This is my first module.

I am looking for productive criticism, code review, and feedback.

Any other pointers and tips are welcome.

Comments

pbrown063 created an issue. See original summary.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Needs review » Needs work

https://cgit.drupalcode.org/sandbox-pbrown063-3026144/tree/LICENSE.txt => remove as the d.o packager adds this.
https://cgit.drupalcode.org/sandbox-pbrown063-3026144/tree/email_domain_... and https://cgit.drupalcode.org/sandbox-pbrown063-3026144/tree/email_domain_... => markup shouldn't be used in translatable strings. That makes it really confusing for translators.
https://cgit.drupalcode.org/sandbox-pbrown063-3026144/tree/email_domain_... => email_domain_validate_mxr_disable => Try deleting things using a wildcard. What if this optional module gets enabled, then later disabled while this module remains configured? We'd have stray variables.

https://cgit.drupalcode.org/sandbox-pbrown063-3026144/tree/email_domain_... => I think this allows for JS nastiness as we don't sanitize the domain. What if that is some juicy XSS?

pbrown063’s picture

pbrown063 commented

Lucas, each of the flags that you raised are resolved by the following set of commits:

remove as the d.o packager adds this.

https://git.drupalcode.org/sandbox/pbrown063-3026144/commit/09ed73b

markup shouldn't be used in translatable strings. That makes it really confusing for translators.

https://git.drupalcode.org/sandbox/pbrown063-3026144/commit/2afb30c
https://git.drupalcode.org/sandbox/pbrown063-3026144/commit/53833d7

Try deleting things using a wildcard.

https://git.drupalcode.org/sandbox/pbrown063-3026144/commit/61f8359

What if that is some juicy XSS?

https://git.drupalcode.org/sandbox/pbrown063-3026144/commit/f766ce7

pbrown063’s picture

pbrown063 commented
Assigned: Unassigned » pbrown063
Status: Needs work » Fixed
pbrown063’s picture

pbrown063 commented

Status: Fixed » Closed (fixed)

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

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Version: » 7.x-1.x-dev
Priority: Major » Normal
Issue tags: -Module review, -