Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved email title #6

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

IQAndreas
Copy link
Contributor

The title of the email sent out by commentsubmit.php has been improved to

Comment from [name] on '[post title]'

making it easier to sort out when it gets to the inbox.

@mpalmer
Copy link
Owner

mpalmer commented Aug 9, 2012

8be08e8 has the same unhealthy obsession with htmlspecialchars in inappropriate places, and 7768c09 is, AFAICT, the same changes from your previous pull request. I do like the idea of a better subject line, though, and I'd encourage you to fix up the "Improved email title" patch to not use htmlspecialchars when you're not rendering HTML.

@IQAndreas
Copy link
Contributor Author

Alrighty then, attached new commits. Is this version better?

@mpalmer
Copy link
Owner

mpalmer commented Aug 12, 2012

What does 8e6b17b have to do with improving the e-mail title? Other problems with that commit:

  • The commit's message doesn't match with the contents of the commit; there are a lot of things other than just adding those two functions.
  • You've eaten a whitespace-after-comma on line 26.
  • If you haven't changed the functionality associated with the comment on lines 24-27, why did you change the comment?
  • I'm not sure that preg_replace is doing what you think it's doing. What were your test cases for that function?
  • sanitizeHeaderField is a poor function name; what sort of "header field" are you sanitising, and what does it mean to "sanitise" such a value in that context?
  • My religion forbids me from dealing with dodgyCamelCase identifiers.
  • You have a typo in getPostField ($defautValue)

For 49bb392:

  • Do you need to escape page.title in any way when placing it into a form field?
  • Is the sanitisation of the fields you're pulling appropriate in the context you're using them?

This seems uneccessary, but will mainly help avoid merge conflicts in future changes.

- `$subject` is defined elsewhere (but immediately set to `$SUBJECT`)
- `$msg` renamed to `$message`. I'm not poor; I can afford 4 extra characters
- `$headers` is a separate variable rather than passed directly as a string
Currently only used in one place, but will be used more in the future.
The header is now properly cleaned to avoid at least one form of email injection (even in case the blog owner accidentally puts a bad email address in the `$EMAIL_ADDRESS` variable.

Both functions taken from http://mattgeri.com/blog/2012/01/escaping-input-to-the-php-mail-function/ (should I include attribution in the code as well?)
@IQAndreas
Copy link
Contributor Author

What does 8e6b17b have to do with improving the e-mail title?

I created that extra commit so the two issues could have a common ancestor; this was for two reasons:

  • There were changes made in 8e6b17b which both issues took advantage of.
  • They both changed the same line of code (the one containing the mail function), which would mean the two separate issues would need to be merged manually rather with the "quick and easy" GitHub button.

I have a third version of this issue as well, but I'll wait until issue #8 is resolved in case there is anything in commits 105d5b3 to 835d290 that still needs improvement.

The truth is, I'm mostly working my way up to the version I'm using on my blog (and as you have noted, still has a few things that need and will be fixed), but taking it one significant change at a time.

I really appreciate you taking the time to go through my changes, making clear what is wrong with them, and suggesting fixes or improvements. I don't get that sort of help when working on my own projects.

The title of the email sent out by `commentsubmit.php` has been improved to "Comment from <name> on '<post title>'" making it easier to sort comments when they get to your inbox.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants