WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 7 months ago

#21730 new enhancement

More modular and reusable email validation functions

Reported by: boonebgorges Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch needs-unit-tests
Focuses: Cc:

Description

Email validation, especially as it's handled in Multisite (most of which is verbatim from MU), is pretty messy. We have some functions like is_email_address_unsafe() for checking banned domains, but we don't have a parallel function for checking against limited_email_domains. There are no filters outside of wpmu_validate_user_signup, which means that if you want to use email validation outside of the normal MS registration workflow and want to tweak the way that it works (see eg #15706, #20459), you pretty much have to roll your own. And there's no single function that a plugin like BuddyPress can use to do all relevant email checks in one fell swoop.

The attached patch suggests the following changes:

  • Put the limited_email_domains check into a function, is_email_address_allowed().
  • Put filters on the output of this new function as well as is_email_address_unsafe().
  • Introduce function wp_validate_email_address(), which wraps the following four checks: is_email(), email_exists(), is_email_address_allowed(), is_email_address_unsafe().
  • Rearranges wpmu_validate_user_signup() a bit so that all email checks (as opposed to username checks) happen together.

I'm not married to anything in this particular implementation (the way that wp_validate_email_address() sends back error messages is not particularly beautiful, but I didn't want to introduce a ton of overhead), but I would really like to see some sort of treatment along these lines, to make things more modular and reusable.

If something like this gets approved by the devs, I would like to further suggest the following:

  • Give a similar treatment to username validation
  • Move the generic validation functions out of ms-functions.php (with function_exists() checks on the MS-specific stuff)

I'm happy to work more on this kind of patch, but didn't want to go too far in case it's a non-starter for some reason.

Attachments (2)

21730.patch (6.0 KB) - added by boonebgorges 4 years ago.
21730.02.patch (13.2 KB) - added by imath 8 months ago.

Download all attachments as: .zip

Change History (8)

@boonebgorges
4 years ago

#1 @johnjamesjacoby
4 years ago

Patch seems fine to me, and I agree it would be nice to get much of this out of ms-functions.php.

#3 @Ipstenu
4 years ago

That would be great. Then I could extend it more easily to let people block by individual email address ( foobar@… vs all of gmail.com ;) ) - Ban Hammer is a little hackarific as is.

#4 @boonebgorges
14 months ago

In 32638:

Improve tests for is_email_address_unsafe().

  • Move to a separate file for better organization and method names.
  • Use a dataProvider when appropriate, for better readability.
  • Add a test for splitting the banned domain list on line breaks.

See #20459, #21730.

#5 @imath
8 months ago

Hi everyone,

I agree with @boonebgorges & @johnjamesjacoby. A bit like in #17904 (which in a way tries to make username validation very similar between regular and ms WordPress), i think it would be nice to have the "ms email checks" available for regular WordPress config.

I can't think of a good reason explaining why a regular config can't restrict registration according to email domains for instance.

So 21730.02.patch is a suggestion to bring the email restrictions into regular configs and to be able to use the functions Boone suggested no matter the WordPress configuration in use.

@imath
8 months ago

#6 @boonebgorges
7 months ago

  • Keywords needs-unit-tests added; dev-feedback removed
  • Milestone changed from Awaiting Review to Future Release

@imath - Thanks for bringing this ticket back to life.

Let's definitely go forward with the original proposal, which is to abstract and consolidate the email validation logic. But let's be good citizens and make sure that the functions have reasonably good test coverage when we introduce them.

I am unsure about introducing the Multisite UI to non-Multisite. The distinction between Limited Email Domains and the domain blacklist is very unclear in the UI, and the way they interact at a lower level is inconsistent and non-obvious. See #15706, and especially the IRC chat linked to at https://core.trac.wordpress.org/ticket/15706#comment:38. I'm not sure that I want to encourage the use of this functionality before resolving some of the UX and flow issues.

Note: See TracTickets for help on using tickets.