Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add filter for plugins to add support link to login form. #615

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
+49 −26
Open

Add filter for plugins to add support link to login form. #615

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7dc05d7
Add a filter so consumers can add links to the problem area.
StevenDufresne Jul 11, 2024
16a1d5c
Document filter.
StevenDufresne Jul 11, 2024
8918127
Pass all the links so we can control link location.
StevenDufresne Jul 12, 2024
5a60522
Refactor to display links outside of backup_providers.
StevenDufresne Sep 23, 2024
61121cd
Update doc block for filter and bump version.
StevenDufresne Sep 23, 2024
5ac8a23
Update documentation and remove <li> from the string.
StevenDufresne Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 22 additions & 10 deletions class-two-factor-core.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -825,17 +825,29 @@ public static function login_html( $user, $login_nonce, $redirect_to, $error_msg
<?php esc_html_e( 'Having Problems?', 'two-factor' ); ?>
</p>
<ul>
<?php
foreach ( $backup_providers as $backup_provider_key => $backup_provider ) :
<?php
$links = [];

foreach ( $backup_providers as $backup_provider_key => $backup_provider ) {
$backup_link_args['provider'] = $backup_provider_key;
?>
<li>
<a href="<?php echo esc_url( self::login_url( $backup_link_args ) ); ?>">
<?php echo esc_html( $backup_provider->get_alternative_provider_label() ); ?>
</a>
</li>
<?php endforeach; ?>
</ul>
$links[] = sprintf(
'<li><a href="$1%s">$2%s</a></li>',
esc_url( self::login_url( $backup_link_args ) ),
esc_html( $backup_provider->get_alternative_provider_label() )
);
}

/*
* 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 );
kasparsd marked this conversation as resolved.
Show resolved Hide resolved
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/*
* 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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This appears to be addressed in the follow-up commits.

Copy link
Member

Choose a reason for hiding this comment

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

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.

?>
</ul>
</div>
<?php endif; ?>

Expand Down