Remove duplicate two_factor_providers filter calls to allow disabling core providers
#651
Conversation
kasparsd
force-pushed
the
fix-available-providers-filter
branch
from
4a9feff to
93f561f
Compare
| foreach ( $providers as $provider_key => $path ) { | ||
| require_once $path; | ||
| if ( ! empty( $path ) && is_readable( $path ) ) { |
There was a problem hiding this comment.
Previously this wasn't checking if the file path even exists. If the path isn't valid, we still attempt to check if the class exists and include it.
This was always a private method so we don't need to account for anyone referencing this.
| */ | ||
| if ( method_exists( $class, 'get_instance' ) ) { | ||
| if ( class_exists( $class ) ) { |
There was a problem hiding this comment.
The get_instance() presence is checked later when we actually attempt to load these classes. This method is strictly for mapping the classes and it shouldn't know anything about their structure.
|
|
||
| foreach ( $providers as $provider_key => $provider_class ) { | ||
| if ( method_exists( $provider_class, 'get_instance' ) ) { |
There was a problem hiding this comment.
Previously it wouldn't actually remove the provider from the list if it didn't have the get_instance method.
| $providers = array_merge( $providers, $additional_providers ); | ||
| } | ||
|
|
||
| foreach ( self::get_providers_classes( $providers ) as $provider_class ) { |
There was a problem hiding this comment.
Here we still pass the provider classes through the filters so any provider adjustments are accounted for.
| unset( $providers[ $provider_key ] ); | ||
| } | ||
| try { | ||
| $providers[ $provider_key ] = call_user_func( array( $provider_class, 'get_instance' ) ); |
There was a problem hiding this comment.
This will fail both when (1) get_instance is not defined for the class and (2) if that methods triggers some other error.
|
@ocean90 This is ready for another round of code review. I've described the approach in the description of the pull request and added a test to cover this new use-case during uninstall. During normal operation the |
| } | ||
| ); | ||
|
|
||
| $this->assertNotContains( 'Two_Factor_Email', array_keys( Two_Factor_Core::get_providers() ), 'Default provider can be disabled via a filter' ); |
|
This has been revised and is ready for code review. I've also added unit tests to confirm that it fixes the issue and allows removing core providers. Would appreciate a code review. |
|
@kasparsd - Thanks for your work on this. I can confirm that it is once again possible to disable a core provider with the |
kasparsd
changed the title
two_factor_providers filter calls to allow disabling core providers
See #637 (comment)
What?
In #637 we introduced a dedicated helper to retrieve all possible two-factor providers (even if disabled) so that we could purge all their added user meta and options during uninstall.
Accidentally it added another instance of the
two_factor_providersfilter which was already defined inget_providers().Why?
How?
two_factor_providersfilter when getting "enabled" providers fromget_providers().get_providers_classes()to accept a list of provider names and their class files instead of attempting to retrieve some provider state. During uninstall, we need all providers while during runtime only the enabled ones.Testing Instructions
two_factor_providersfilters. All default providers are available to all users.two_factor_providersthat limits just one provider. Attempt to uninstall the plugin and confirm that it removes also the meta for the disabled providers (see the phpunit test case cover this).Screenshots or screencast
Changelog Entry
Changed - removed duplicate two-factor provider filter.