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.

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

Workaround to avoid crashing bbPress' frontend profile page #414

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

CryoutCreations
Copy link

@CryoutCreations CryoutCreations commented Sep 3, 2021

This workarounds skips the addition of the TwoFactor fields to the user profile when it's part of bbPress' frontend profile edit page. These are settings that shouldn't probably by editable on the forum profile anyway.

Fixes the Fatal error: Uncaught Error: Call to undefined function convert_to_screen() reported in #175

Frontend profile page error example:
Error example

@@ -925,6 +925,11 @@ public static function manage_users_custom_column( $output, $column_name, $user_
* @param WP_User $user WP_User object of the logged-in user.
*/
public static function user_two_factor_options( $user ) {

/* workaround for two factor options crashing bbpress' profile page */
if ( function_exists('bbp_is_user_home_edit') && bbp_is_user_home_edit() ) return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is currently relying on a specific integration and historically this plugin doesn't have any conditional logic to account for specific integrations. Could we somehow convert this into something that can be placed in the class-two-factor-compat.php file which currently deals with Jetpack specific things?

https://github.com/WordPress/two-factor/blob/master/class-two-factor-compat.php

Which of the functionality in this method is actually not compatible with the front-end output?

Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to check out my comment where I outline frontend integration with this problem (and a few more) here: #335 (comment)

For this specific issue, it would probably be better to redeclare the admin functions on the frontend when needed.

Copy link
Author

Choose a reason for hiding this comment

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

This is currently relying on a specific integration and historically this plugin doesn't have any conditional logic to account for specific integrations. Could we somehow convert this into something that can be placed in the class-two-factor-compat.php file which currently deals with Jetpack specific things?

https://github.com/WordPress/two-factor/blob/master/class-two-factor-compat.php

Which of the functionality in this method is actually not compatible with the front-end output?

I updated the PR to include an example error screenshot.
Until the required admin functions are exposed on the frontend (should they?), it might be possible to rewrite the workaround in the compat file to conditionally remove the two action hooks instead. I'll look into this.

@jeffpaul jeffpaul requested a review from kasparsd December 23, 2021 15:11
@jeffpaul jeffpaul added this to the 0.8.0 milestone Dec 23, 2021
@jeffpaul jeffpaul linked an issue Dec 23, 2021 that may be closed by this pull request
Zed added 2 commits December 23, 2021 22:39
…o-factor-compat class;

Also changed function check from bbp_is_user_home_edit() to bbp_is_single_user_edit() as this is more generally used
@CryoutCreations
Copy link
Author

Per @kasparsd's request I moved the bbPress conditionality check to the two-factor-compat class and changed the function call to use a more general function.

Hopefully the code now meets the logic separation requirements.

@jeffpaul
Copy link
Member

Per yesterday's bug scrub, while we want to be compatible with official plugins we're going to punt this to a future release so that the 0.8.0 release can focus on the U2F deprecation.

@jeffpaul jeffpaul modified the milestones: 0.8.0, Future Release Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Breaks bbPress profile editing
4 participants