Add filter for plugins to add support link to login form.#615
Add filter for plugins to add support link to login form.#615StevenDufresne wants to merge 10 commits intoWordPress:masterfrom
Conversation
class-two-factor-core.php
Outdated
| /* | ||
| * Allow plugins to add links to the two-factor login form. | ||
| */ | ||
| $links = apply_filters( 'two_factor_login_support_links', $links ); | ||
|
|
||
| // Echo out the filtered links | ||
| foreach ( $links as $link ) { | ||
| echo wp_kses_post( $link ); | ||
| } |
There was a problem hiding this comment.
| /* | |
| * Allow plugins to add links to the two-factor login form. | |
| */ | |
| $links = apply_filters( 'two_factor_login_support_links', $links ); | |
| // Echo out the filtered links | |
| foreach ( $links as $link ) { | |
| echo wp_kses_post( $link ); | |
| } | |
| /* | |
| * Allow plugins to filter the backup method links on the login form. | |
| */ | |
| $links = apply_filters( 'two_factor_login_backup_links', $links ); | |
| // Echo out the filtered links | |
| echo implode( '', $links ); |
Given this filter only runs when there are backup providers, calling it a support links might be confusing for some, we can probably name it something like two_factor_login_backup_links?
There's probably an argument that this should be built prior to the if ( $backup_providers ) conditional to output filtered data even when there exist no backup methods for the given user?
I don't think we need to explicitly sanitize the HTML here either, it's either hard-coded HTML or from a PHP function that can do anything anyway.
There was a problem hiding this comment.
This appears to be addressed in the follow-up commits.
There was a problem hiding this comment.
These points remain:
Given this filter only runs when there are backup providers, calling it a support links might be confusing for some, we can probably name it something like two_factor_login_backup_links?
OR
There's probably an argument that this should be built prior to the if ( $backup_providers ) conditional to output filtered data even when there exist no backup methods for the given user?
I suspect the intention of the filter was actually to be used when the user has no backup provider, or, at least that others who would have a use for it would be in that group.
b658956 to
8918127
Compare
kasparsd
left a comment
There was a problem hiding this comment.
@StevenDufresne Was there anything else you wanted to include here or is this ready for merge?
If you don't get around to it, we'll extend the inline docblock for the filter to include documentation for the arguments (like here) and also add it to the relevant readme section.
| <?php | ||
| foreach ( $links as $link ) { | ||
| echo '<li>' . $link . '</li>'; | ||
| } | ||
| ?> |
There was a problem hiding this comment.
I'd far sooner continue the prior pattern of late escaping the url and text for clarity's sake. How about instead of a link, we pass through an array of 'url' and 'label' keyed to slot in?
Or, far more simply, as backup_providers is literally only used to output these links in this function, why not rename and filter that variable instead and leave the prior infastructure in place?
|
Thanks @georgestephanis for the review. @StevenDufresne Sorry for the long waiting time. Would you be able to check the comments and provide an updated PR? We'd be happy to merge this asap :) |
|
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 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. |
Co-authored-by: George Stephanis <daljo628@gmail.com>
|
@masteradhoc I'm not certain if I'll be able to get to this shortly. Apart my now dated desire for this feature, are there any other reasons for this to become a high priority for me? |
masteradhoc
left a comment
There was a problem hiding this comment.
@georgestephanis per your comment i've adjusted the code and suggested several adjustments. Feel free to commit them if they are fine for you.
Im able to use the filter perfectly in this way
add_filter( 'two_factor_login_backup_links', 'twentytwentyfive_two_factor_login_backup_links' );
function twentytwentyfive_two_factor_login_backup_links( $links ) {
$links[] = array(
'url' => 'https://google.com',
'label' => __( 'Contact the support team', 'twentytwentyfive' ),
);
return $links;
}
| } | ||
| if ( $interim_login ) { | ||
| $backup_link_args['interim-login'] = 1; | ||
| $links = []; |
There was a problem hiding this comment.
| $links = []; | |
| $links = array(); |
rework to array as per @georgestephanis comment
| $backup_link_args['interim-login'] = 1; | ||
| $links = []; | ||
|
|
||
| if ( $backup_providers ) { |
There was a problem hiding this comment.
| if ( $backup_providers ) { | |
| if ( $backup_providers ) { |
remove unneeded doublespace
| $links[] = sprintf( | ||
| '<a href="%1$s">%2$s</a>', | ||
| esc_url( self::login_url( $backup_link_args ) ), | ||
| esc_html( $backup_provider->get_alternative_provider_label() ) | ||
| ); |
There was a problem hiding this comment.
| $links[] = sprintf( | |
| '<a href="%1$s">%2$s</a>', | |
| esc_url( self::login_url( $backup_link_args ) ), | |
| esc_html( $backup_provider->get_alternative_provider_label() ) | |
| ); | |
| $links[] = array( | |
| 'url' => self::login_url( $backup_link_args ), | |
| 'label' => $backup_provider->get_alternative_provider_label(), | |
| ); |
rework as per @georgestephanis comment
| ?> | ||
|
|
||
| /** | ||
| * Filters the html links displayed on the two-factor login form. |
There was a problem hiding this comment.
| * Filters the html links displayed on the two-factor login form. | |
| * Filters the links displayed on the two-factor login form. |
small clarification
| * | ||
| * @since 0.16.0 | ||
| * | ||
| * @param array<string> $links An array of links displayed on the two-factor login form. |
There was a problem hiding this comment.
| * @param array<string> $links An array of links displayed on the two-factor login form. | |
| * @param array $links An array of links displayed on the two-factor login form, each with `url` and `label` keys. |
| <?php endforeach; ?> | ||
| <?php | ||
| foreach ( $links as $link ) { | ||
| echo '<li>' . $link . '</li>'; |
There was a problem hiding this comment.
| echo '<li>' . $link . '</li>'; | |
| echo '<li><a href="' . esc_url( $link['url'] ) . '">' . esc_html( $link['label'] ) . '</a></li>'; |
What?
Sometimes users have difficulty with their two-factor settings. In the "Having Problems?" section, we currently don't have a way to add links that are not providers.
This PR adds a filter called
two_factor_login_support_linksto allow plugins to provide non-provider support links.Why?
We want to give users access to documentation that can help them with using 2fa or with recovering their account.
For example, it would allow us to do
How?
It adds a filter inside the
<ul>so HTML can be passed in like so:Testing Instructions
Changelog Entry