Update email provider token-message hooks - #897
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @christianc1. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Pull request overview
This PR updates the Two_Factor_Email provider’s token email subject/message filter hooks to expose the $token in the subject filter, introduce consistently named replacement hooks (two_factor_email_token_*), and deprecate the legacy two_factor_token_email_* hooks while adding corresponding tests and documentation updates.
Changes:
- Deprecates
two_factor_token_email_subject/two_factor_token_email_messageand introducestwo_factor_email_token_subject/two_factor_email_token_messagewith symmetric argument shapes. - Adds PHPUnit coverage for both deprecated and new hooks.
- Documents the new hooks in
readme.txt.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
providers/class-two-factor-email.php |
Adds new two_factor_email_token_* filters and wraps legacy hooks in apply_filters_deprecated(). |
tests/providers/class-two-factor-email.php |
Adds tests validating subject/body filtering through both deprecated and new hooks. |
readme.txt |
Documents the new email token subject/body filters (and minor whitespace cleanup nearby). |
Comments suppressed due to low confidence (1)
providers/class-two-factor-email.php:343
- The deprecation notice version for this hook doesn't match the
@deprecated 0.17.0tag above.apply_filters_deprecated()should use the version where the hook was deprecated so the emitted message is accurate.
$message = apply_filters_deprecated( 'two_factor_token_email_message', array( $message, $token, $user->ID ), '0.11.0', 'two_factor_email_token_message' );
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function () { | ||
| return 'New Subject'; | ||
| } |
There was a problem hiding this comment.
Adding $subject to the callback triggers a phpcs warning:
451 | WARNING | The method parameter $subject is never used (Generic.CodeAnalysis.UnusedFunctionParameter.Found)
So line 449 would actually need to be:
function ( $subject ) { // phpcs:ignore Generic.CodeAnalysis.UnusedFunctionParameter.Found Similar to
Line 180 in 6d70339
masteradhoc
left a comment
There was a problem hiding this comment.
Thank you very much @joemaller for the PR, we appreciate it!
The missing $token in the subject filter is a real gap, and the use case (putting the code in the subject line) is a good one!
That said, I'd rather we avoid the deprecation and rename here, and just append $token as the last argument to the existing filter:
This is fully backwards compatible, so nothing existing changes behaviour. It gets us the actual goal of the PR (developers can put the token in the subject) in one line, with no new hook names to maintain.
I know the argument order then doesn't line up with two_factor_token_email_message ($subject, $user_id, $token vs $message, $token, $user_id), and that isn't pretty. But inconsistent argument order is a documentation problem, whereas deprecating a public hook is a permanent maintenance obligation — in practice we'd be carrying both names indefinitely. Core makes this same trade-off regularly for the same reason.
The two_factor_token_email_message rename is the part I'd most likely drop from this PR. I agree two_factor_email_token_* is the more consistent namespace given two_factor_email_token_ttl and _length, but I don't think that's worth spending a deprecation on.
What do you think @georgestephanis ?
@joemaller Would you be able to update the PR along those lines when you get a chance, and pick up the Copilot review feedback and test adjustments at the same time? Happy to take another look once it's pushed.
|
Hi @masteradhoc, thanks for the review. Yes, a one-line change to append Some of the considerations behind the proposed changes:
There is an expectation that Two Factor will eventually land in WordPress core, many of us have standardized on it for that reason. Feature plugins exist to get the public surface as clean as possible before moving to core. A future merge would also be a natural opportunity to drop any lingering deprecations. Even if the old hooks stick around forever, this cognitive and aesthetic improvement is worth the small cost. Millions of sites will likely use this, let's give those developers an elegant solution. |
Thanks for letting me know about the proposed change. In the past, the two-factor plugin has removed PHP class methods outright without deprecating or changed CSS selectors in the markup and I've had to adjust my code to fix some issues so I appreciate the ping. |
|
@joemaller Just noting that a public search of Github (AFAIK) doesn't surface private repositories, so usage signals derived by that method should be taken with a grain of salt. Also, kicking myself that I didn't see the use case you described here 6 years ago when I contributed the filter, it's a great idea for U/X. |
|
Thank you @joemaller for your feedback. While we check this can you fix those PHP lint issues? Please make sure your code passes phpstan v5 - which is what we're aming for in the next release :) |
|
@masteradhoc all linting errors and Copilot suggestions resolved. Thank you for merging master back onto this. |
|
@christianc1, No, search doesn't include private repos, I should've left the laughing emoji in that point. But I don't think those results are an entirely meaningless metric. Also I tagged you specifically since you wrote the original and there was a good chance you all were using it internally (in private repos). Thanks for adding the filter in here in the first place! |
What?
The primary purpose of this PR is to provide the
$tokento the email provider's subject filter. It also brings consistency to the shape of Two Factor's email provider token-message hook arguments, and standardizes the namespaces used by those filters. Includes tests and documentation.Fixes: #898
Why?
Many services now include the login token in their email subject lines like "Your login code is 123456". This improves user-experience and speeds MFA logins.
Previously, the email subject filter did not have access to
$tokenand its arguments did not match the shape of the message filter arguments. Developers wanting to include the token in email subjects had to use clumsy, fragile workarounds.This PR adds (renames) two hooks:
two_factor_email_token_subjecttwo_factor_email_token_messageThis PR deprecates two existing hooks:
two_factor_token_email_subjecttwo_factor_token_email_messageNew hook names provide a clean pathway for changing the signature of
two_factor_token_email_subjectwithout breaking existing functionality. The new filter adds a$tokenargument to match the shape oftwo_factor_token_email_message. This makes it very easy for developers to include$tokenin email subject lines.How?
two_factor_email_token_subjectandtwo_factor_email_token_messagefilters are now symmetrical:$subject|$message, $token, $user_id.Use of AI Tools
AI assistance: Yes
Tool(s): Opencode, browsers
Model(s): grok 4.3, gemini-3.1-pro-preview
Used for: Architectural suggestions, consistency and style-matching with existing code, and code review/QA.
Testing Instructions
Tests covering the changes were added.
Add this line to a theme with Two Factor installed to add the token to the email subject.
Changelog Entry