Skip to content

Contact Form: Add display names to addresses in the To header#6657

Merged
zinigor merged 2 commits intomasterfrom
fix/html-no-brackets
Apr 24, 2017
Merged

Contact Form: Add display names to addresses in the To header#6657
zinigor merged 2 commits intomasterfrom
fix/html-no-brackets

Conversation

@iandunn
Copy link
Copy Markdown

@iandunn iandunn commented Mar 14, 2017

This avoids SpamAssassin's TO_NO_BRKTS_HTML_ONLY rule.

See #6114

Testing instructions:

  • From master, fill out a [contact-form] and copy the email source to http://spamcheck.postmarkapp.com/
  • Note the triggered TO_NO_BRKTS_HTML_ONLY rule
  • Check out this branch, and re-run the test. The rule should no longer be triggered.

@jeherve jeherve added [Feature] Forms [Status] Needs Review This PR is ready for review. Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Pri] Normal labels Mar 15, 2017
@iandunn
Copy link
Copy Markdown
Author

iandunn commented Mar 15, 2017

It looks like the test failures in Travis are unrelated.

@georgestephanis
Copy link
Copy Markdown
Contributor

georgestephanis commented Mar 16, 2017

I'm rerunning the failed 7.0 test

edit: rerun tests worked fine.


foreach ( $to as $to_key => $to_value ) {
$to[ $to_key ] = Grunion_Contact_Form_Plugin::strip_tags( $to_value );
$to[ $to_key ] = self::add_name_to_address( $to_value );
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth attempting to see if there is a "Name" field, and if so, use that instead of the email address for the name portion?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, that's a good idea. I'll add that :)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Er, nevermind, now I remember why I didn't do that. We don't actually have a good source for the $to name, and it seems unlikely that users would take advantage of the ways it could be made available. It comes from these sources:

  1. The [contact-form to="" parameter. The form builder will remove anything wrapped in < or > fields when inserting the shortcode, so it would break an address with a name part. I guess somebody could manually add it a name to the parameter, but that seems like a pretty small edge case, and it'd break if they ever used the form builder to modify the shortcode. add_name_to_address() does make sure that it only adds a name if it's passed a raw address, so it should work if anyone tries it, as long as they don't use the form builder.
  2. get_option( 'admin_email' ). We could try to fetch any WP_User accounts with a matching email address, then pull that user's name, but it won't always work since admin_email is independent of user accounts. It wouldn't hurt anything, it could just fall back to using the address as the name if it didn't, but I'm not sure it's worth it. I'm happy to add it if you feel like it is, though.
  3. The contact_form_to filter, which also seems like a small edge case. I don't think many people will interpret the filter docs as saying that a name part would be allowed, but it's possible. add_name_to_address() wouldn't modify the address if it were included, so this should already work.

Right now the To message header is the only place that's using add_name_to_address(), since the From and Reply-To headers are already built with names.

@samhotchkiss samhotchkiss added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Mar 28, 2017
@samhotchkiss samhotchkiss self-requested a review March 28, 2017 03:43
Copy link
Copy Markdown
Contributor

@samhotchkiss samhotchkiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me-- kind of silly, but if it gets us better deliverability, then I'm all for it.

@iandunn
Copy link
Copy Markdown
Author

iandunn commented Mar 28, 2017

a505b8b merges master and fixes the conflicts.

iandunn added 2 commits April 24, 2017 12:35
This avoids SpamAssassin's `TO_NO_BRKTS_HTML_ONLY` rule.

See #6114
Display names were added to addresses in d224e4f, and the unit tests should have been updated at that time.
@zinigor zinigor force-pushed the fix/html-no-brackets branch from 3a5215e to b6067c8 Compare April 24, 2017 18:35
@zinigor
Copy link
Copy Markdown
Contributor

zinigor commented Apr 24, 2017

Rebased to fix conflicts and avoid merge commits.

@zinigor
Copy link
Copy Markdown
Contributor

zinigor commented Apr 24, 2017

Tested, works well for me!

@zinigor zinigor merged commit 3181831 into master Apr 24, 2017
@zinigor zinigor deleted the fix/html-no-brackets branch April 24, 2017 19:03
@zinigor zinigor removed the [Status] Ready to Merge Go ahead, you can push that green button! label Apr 24, 2017
jeherve added a commit that referenced this pull request Apr 24, 2017
eliorivero pushed a commit that referenced this pull request Apr 25, 2017
* Changelog: initial commit for 4.9 release.

* Changelog: add #6929

* Changelog: move old changelogs to changelog.txt

* Readme: restore deleted release post link.

The post is now live.

* Changelog: add #6853

* Changelog: add #6856

* Changelog: add #6857

* Changelog: add #6884

* Changelog: add #6885

* Changelog: add #6892

* Changelog: add #6894

* Changelog: add #6898

* Changelog: add #6899

* Changelog: add #6900

* Changelog: add #6909

* Changelog: add #6927

* Changelog: add #6947

* Chagelog: add #6958

* Changelog: add #6961

* Changelog: add #6963

* Changelog: add #6965

* Changelog: add #6986

* Changelog: add #7000

* Changelog: add #7013

* Changelog: add #7015

* Changelog: add #7019

* Changelog: add #7028

* Changelog: add #6998

* Changelog: add #6999

* Changelog: add #7044

* Changelog: add #6881

* Changelog: add #6922

* Changelog: add #6940

* Changelog: add #6962

* Changelog: add #6942

* Changelog: add #6959

* Changelog: add #7018

* Changelog: add #6948

* Changelog: add #6657

* Changelog: add #7030

* Changelog: add #7048

* Changelog: add #7031

* Changelog: add #6990

* Changelog: add #6957

* Changelog: add #7027
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Forms [Pri] Normal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants