jekyll-static-comments icon indicating copy to clipboard operation
jekyll-static-comments copied to clipboard

Improved email title

Open IQAndreas opened this issue 12 years ago • 4 comments

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.

IQAndreas avatar Jun 27 '12 05:06 IQAndreas

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.

mpalmer avatar Aug 09 '12 09:08 mpalmer

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

IQAndreas avatar Aug 11 '12 22:08 IQAndreas

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?

mpalmer avatar Aug 12 '12 04:08 mpalmer

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.

IQAndreas avatar Aug 16 '12 04:08 IQAndreas