[Fix] Deprecated Noticed on Admin Setup Wizard #2414
Conversation
osmansufy
added
the
Needs: Dev Review
WalkthroughThe changes in Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
includes/Admin/SetupWizard.php (3)
Line range hint
1019-1029: Improve withdraw limit validation.The withdraw limit validation could be strengthened to ensure proper numeric handling and prevent potential issues.
Consider this improved validation:
-if ( ! empty( $_POST['withdraw_limit'] ) ) { - $input_limit = sanitize_text_field( wp_unslash( $_POST['withdraw_limit'] ) ); - $options['withdraw_limit'] = is_numeric( $input_limit ) && $input_limit >= 0 ? wc_format_decimal( $input_limit ) : 0; -} else { - $options['withdraw_limit'] = 0; -} +$input_limit = isset( $_POST['withdraw_limit'] ) ? sanitize_text_field( wp_unslash( $_POST['withdraw_limit'] ) ) : ''; +$options['withdraw_limit'] = is_numeric( $input_limit ) && $input_limit >= 0 ? wc_format_decimal( $input_limit ) : 0;
Line range hint
1264-1277: Consider adding nonce verification for plugin installation.The plugin installation process should include additional security checks to prevent unauthorized installations.
Add nonce verification before proceeding with plugin installation:
protected function install_plugin( $plugin_id, $plugin_info ) { + if ( ! current_user_can( 'install_plugins' ) || ! wp_verify_nonce( $_REQUEST['_wpnonce'], 'dokan-setup' ) ) { + wp_die( esc_html__( 'You are not authorized to perform this action.', 'dokan-lite' ) ); + } + // Make sure we don't trigger multiple simultaneous installs. if ( get_option( 'woocommerce_setup_background_installing_' . $plugin_id ) ) { return; }
Based on the gathered information, I can now provide a final response about the race condition concern.
The race condition concern is valid and the suggested solution using transients is appropriate
The current implementation has these potential issues:
- Uses permanent options instead of transients for temporary installation flags
- The cleanup of the installation flag happens in a shutdown hook, which might not execute if the process fails
- No timeout mechanism exists for stale installation flags
The suggested solution using WordPress transients is better because:
- Transients have built-in expiration, automatically cleaning up stale flags
- They're designed for temporary data storage
- The 5-minute timeout provides a safety net if the cleanup code doesn't execute
- They use the object cache when available, providing better performance
🔗 Analysis chain
Line range hint
1267-1269: Potential race condition in background installation check.The current implementation of preventing multiple simultaneous installs might be susceptible to race conditions.
Let's verify if there are any existing issues with concurrent installations:
Consider using WordPress transients with a short expiration time instead of options for the installation flag:
-if ( get_option( 'woocommerce_setup_background_installing_' . $plugin_id ) ) { +if ( get_transient( 'woocommerce_setup_background_installing_' . $plugin_id ) ) { return; } -update_option( 'woocommerce_setup_background_installing_' . $plugin_id, true ); +set_transient( 'woocommerce_setup_background_installing_' . $plugin_id, true, 5 * MINUTE_IN_SECONDS );
Let me gather more information about how this background installation process works and if there are any error handling mechanisms in place.
Let me check one more thing to understand how the deferred actions and shutdown hooks are implemented for the background installation process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any reported issues or error handling related to concurrent plugin installations rg -A 5 "woocommerce_setup_background_installing_"Length of output: 1295
Script:
#!/bin/bash # Search for the background installation method and related error handling ast-grep --pattern 'function install_plugin_background($$$)' # Look for any cleanup or error handling related to these installation flags rg -B 2 -A 5 "delete_option.*woocommerce_setup_background_installing" # Check if there are any other places where plugin installation status is checked rg -B 2 -A 5 "is_plugin_active.*plugin_info"Length of output: 1227
Script:
#!/bin/bash # Look for the deferred actions implementation and shutdown hook handling ast-grep --pattern 'function run_deferred_actions() { $$$ }' # Check if there are any error handlers or status checks during installation rg -B 2 -A 10 "class SetupWizard"Length of output: 3900
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- includes/Admin/SetupWizard.php (1 hunks)
🔇 Additional comments (1)
includes/Admin/SetupWizard.php (1)
273-280: LGTM! Proper fix for deprecated notices.The changes correctly address the deprecated notices by explicitly enqueueing required WordPress styles before the action hooks. The order of operations is correct:
- Enqueue emoji styles
- Print admin styles
- Enqueue admin bar header styles
- Execute admin head actions
includes/Admin/SetupWizard.php
Outdated
| <?php wp_print_scripts(); ?> | ||
| <?php do_action( 'admin_print_styles' ); ?> | ||
| <?php do_action( 'admin_head' ); ?> | ||
| <?php | ||
| wp_enqueue_emoji_styles(); | ||
| do_action( 'admin_print_styles' ); | ||
| ?> | ||
| <?php | ||
| wp_enqueue_admin_bar_header_styles(); | ||
| do_action( 'admin_head' ); | ||
| ?> | ||
| <?php do_action( 'dokan_setup_wizard_styles' ); ?> |
There was a problem hiding this comment.
Pls put all PHP codeblock inside a single php tag.
There was a problem hiding this comment.
Working on it bhai
…r-Setup-Wizard-#3303
There was a problem hiding this comment.
- Pls fix the PHPCS issue in
includes/Admin/SetupWizard.php - Be careful, You also changed
includes/Vendor/SetupWizard.phpin [Skip] Payment from Setup wizard when no payment method is active #2434. You could merge the changes in one PR if the scopes are same.
| @@ -319,7 +321,7 @@ public function dokan_setup_store() { | |||
| <input type="text" id="calc_shipping_state" name="address[state]" value="<?php echo esc_attr( $address_state ); ?>" / placeholder="<?php esc_attr_e( 'State Name', 'dokan-lite' ); ?>"> | |||
| <span class="error-container"> | |||
| <?php | |||
| if ( ! empty( $_POST['error_address[state]'] ) ) { | |||
| if ( ! empty( $_POST['error_address[state]'] ) ) { // phpcs:ignore | |||
There was a problem hiding this comment.
Assign the $_POST to a variable like $request_data to use safely instead of ignoring PHPCS everywhere.
mrabbani
added
Needs: Author Reply
and removed
Needs: Dev Review
All Submissions:
Changes proposed in this Pull Request:
Related Pull Request(s)
Closes
How to test the changes in this Pull Request:
Changelog entry
Title
Detailed Description of the pull request. What was previous behaviour
and what will be changed in this PR.
Before Changes
Describe the issue before changes with screenshots(s).
After Changes
Describe the issue after changes with screenshot(s).
Feature Video (optional)
Link of detailed video if this PR is for a feature.
PR Self Review Checklist:
FOR PR REVIEWER ONLY:
Summary by CodeRabbit
Bug Fixes
Chores