Hide payment methods that have missing dependencies and display an error in the admin #3920
Conversation
Aljullu
added
status: needs review
type: enhancement
|
Size Change: +248 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
| $payment_method_scripts = $this->payment_method_registry->get_all_active_payment_method_script_dependencies(); | ||
|
|
||
| foreach ( $payment_method_scripts as $payment_method_script ) { | ||
| $deps = $wp_scripts->registered[ $payment_method_script ]->deps; |
There was a problem hiding this comment.
Is this array item guaranteed to exist or do you need a guard?
There was a problem hiding this comment.
Added a guard in 1845b12.
src/Payments/Api.php
Outdated
| * an error in the admin. | ||
| */ | ||
| public function verify_payment_methods_dependencies() { | ||
| global $wp_scripts; |
There was a problem hiding this comment.
I think we can avoid the global by calling $wp_scripts = wp_scripts();?
There was a problem hiding this comment.
Yup, much better. Changed in 5d46dbe.
src/Payments/Api.php
Outdated
| error_log( // phpcs:ignore | ||
| sprintf( | ||
| /* translators: 1: handle of the payment gateway. 2: handle of the dependency that is missing. */ | ||
| esc_html__( 'Payment gateway with handle %1$s has been deactivated because its dependency %2$s is not registered.', 'woo-gutenberg-products-block' ), |
There was a problem hiding this comment.
Do we need to localise debug strings? This is extra burden for translators, and we need to make sense of logs.
There was a problem hiding this comment.
Makes sense not localizing them. Changed in 23a2631.
src/Payments/Api.php
Outdated
| esc_html( $payment_method_script ), | ||
| '<code>' . esc_html( $dep ) . '</code>' | ||
| ); | ||
| if ( defined( 'WP_DEBUG' ) && WP_DEBUG ) { |
There was a problem hiding this comment.
Why are we only showing docs with DEBUG on?
There was a problem hiding this comment.
The idea was not to bother merchants with a link that was only useful to payment method developers. But now that we no longer use a notice in the admin, this is not relevant anymore and we can print the URL in all logs. 👍
src/Payments/Api.php
Outdated
| ) | ||
| ); | ||
| add_action( | ||
| 'admin_notices', |
There was a problem hiding this comment.
Is this the best place to surface errors? I am aware that notices are easy to add but I think it does add a lot of noise within admin.
I think debug log is good, and perhaps we could add an inline script to log to the console too. Those would be visible by developers and it avoids polluting admin notices.
If that works well we should do the same for the circular dependency notice too.
There was a problem hiding this comment.
Good idea! I moved this error from being a notice to a console.error() in a9bd620. For now, I didn't change the circular dependency notice, we can take care of it in a follow-up and maybe even creating a util so we can easily log in the browser console from PHP code.
|
Thanks for the review @mikejolley. I applied your feedback, this is ready for another review. |
mikejolley
deleted the
fix/3800-remove-payment-methods-with-missing-dependencies
branch
Fixes #3800.
In some occasions, a payment method might be registered but its dependencies might not (read #3800 for a more in-depth explanation). In those cases, this PR makes it so:
wp-adminanderror_log()'s the error. IfWP_DEBUGis set to true, it also includes a link to our docs.How to test the changes in this Pull Request:
There are two different set-ups that you can try:
stripeis not registered.public function get_payment_method_script_handles() { $this->asset_api->register_script( 'wc-payment-method-cheque', 'build/wc-payment-method-cheque.js', + [ 'missing-dependency' ] ); return [ 'wc-payment-method-cheque' ]; }wp-adminpage and verify an error notice is displayed for each faulty payment method:Changelog