🚀 enhance: Dokan single store endpoint translation support #94
Conversation
WalkthroughThis pull request introduces comprehensive enhancements to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
dokan-wpml.php (6)
1327-1350: Validate error handling withinregister_single_store_default_endpoint.
The method uses a try-catch block to log any exceptions when callingicl_register_string. This is a good practice. Consider adding a user-facing admin notice if you want to inform admins about failed registrations (in addition to the log).
1352-1379: Correct the spelling in the doc block and confirm rewrite flushing.
Spotted a small spelling issue: "Register Single Store Custome Endpoint." It should be “Custom.” Also, after registering a new custom endpoint, you might consider flushing rewrite rules—either here or in a dedicated callback—to immediately reflect the change, unless your logic callsflush_rewrite_rules()elsewhere.- * Register Single Store Custome Endpoint. + * Register Single Store Custom Endpoint.
1399-1438: Evaluate the performance impact offlush_rewrite_rules().
Callingflush_rewrite_rules()is quite expensive if called frequently. Confirm that this action (dokan_rewrite_rules_loaded) isn’t triggered on every page load, to avoid performance overhead. If needed often, consider deferring this flush until plugin activation or next admin request.
1462-1482: Confirm the trigger frequency forhandle_single_store_endpoint_translation_update.
This method callshandle_translated_single_store_endpoint_rewrite(), which performs rewrite rule management and then a flush. If WPML frequently triggerswpml_st_add_string_translation, you may risk frequent flushes. Consider a more throttle-friendly approach.
1506-1527: Validate potential empty results inget_original_string_value.
Ifget_varreturns null (e.g., no matching row), you might want to log or handle that scenario more distinctly, especially if higher-level methods depend on a non-null string.
1528-1549: Consider defining'dokan-single-store'as a constant inget_original_endpoint_name.
You reference'dokan-single-store'in multiple methods. Using a named constant (e.g.,ENDPOINT_NAME = 'dokan-single-store') may help maintain consistency if you change the endpoint name in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dokan-wpml.php(2 hunks)
🔇 Additional comments (5)
dokan-wpml.php (5)
172-178: Ensure proper hook sequencing and error handling.
The actions and filter hooks for registering and translating single-store endpoints look correctly placed. However, verify that other plugin dependencies (like WPML and Dokan) have completed their initialization before these hooks run. Otherwise, the string registration or rewrite rules might fail silently if the dependencies aren’t fully hooked yet.
1381-1398: Beware of multiple occurrences of the slug inget_translated_single_store_endpoint.
The straightforwardstr_replace()may replace more than one occurrence if$custom_url_slugaccidentally appears multiple times in$store_url. In most real cases, it should be safe, but keep this scenario in mind if expansions to store URLs occur.
1440-1461: Check regex correctness inadd_rewrite_rules.
In the rule'/toc?', the?at the end is treated as a quantifier, not a literal question mark. If you intend to match an optional trailing slash or something similar, ensure the pattern is escaped or formed correctly to avoid unexpected behavior.
1484-1505: Ensure table existence inget_original_string_id.
This direct SQL query presumes{$wpdb->prefix}icl_string_translationsalways exists. While valid in typical WPML setups, consider gracefully handling scenarios where WPML might not be fully installed or if the table prefix differs.
1551-1570: Potential concurrency of endpoint name checks inis_endpoint_registered_with_wpml.
The query looks acceptable. Just be aware that if multiple admin sessions simultaneously register the same endpoint, you might log duplicates. In practice, WordPress concurrency is low, so this is minor. No immediate fix needed, but it’s worth noting.
…ty endpoint value
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
dokan-wpml.php (4)
172-178: Use conditional checks and error handling for translated endpoint hooks.
These hooks look correct for initiating translation logic. Consider adding additional checks to gracefully handle cases where WPML is inactive or not fully loaded, and ensure errors are logged for easier debugging.
1327-1350: Add a user-facing fallback or admin notice on registration failure.
This method cleanly checks if the endpoint is already registered. However, the code only logs failures in the catch block. It might be user-friendly to display an admin notice if registration fails, ensuring admins see the issue and can act on it.
1352-1380: Fix small typo in the doc block ("Custome" → "Custom").
The doc block has a minor spelling issue. Additionally, the fallback pattern here is similar to the default endpoint registration. Consider the same approach of optionally notifying admins on errors.
1399-1439: Avoid frequent flush_rewrite_rules calls for performance.
Flushing rewrite rules can be costly. If this logic is triggered often (e.g., on every page load), consider restricting it to plugin activation or explicit settings updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dokan-wpml.php(2 hunks)
🔇 Additional comments (7)
dokan-wpml.php (7)
1381-1398: Ensure partial-matching edge cases won't cause unexpected string replacements.
Usingstr_replaceis straightforward, but be mindful if$custom_url_slugappears multiple times in$store_urlor as a substring of another slug. For now, it seems correct for typical usage.
1440-1460: Rewrite rules seem correct; confirm no overlap with other endpoints.
These rewrite rules are well formed. Verify that they do not unintentionally override other routes in the same position.
1462-1486: Centralize logic for rewriting endpoints.
This method delegates rewriting to another method, ensuring good separation of concerns. The overall flow is clear. No issues found.
1487-1508: SQL query is properly parameterized.
The query uses$wpdb->prepare, mitigating SQL injection concerns. Performance should be acceptable given that it’s a straightforward lookup.
1509-1530: Return type and prepared statement are consistent.
Good use of nullable return types and$wpdb->prepare. This method is concise and should perform reliably.
1531-1553: Properly filters the original endpoint name by context.
This approach aligns well with WPML’s table structure. The code uses parameterized queries, which is secure.
1554-1573: Ensure unique name usage in WPML strings.
The method checks if a string is already registered by counting records in the WPML strings table. If your naming scheme is consistent, this is fine. No issues noted.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
dokan-wpml.php (5)
1347-1374: Enhance error handling for endpoint registration.While the implementation is functional, the error handling could be improved:
- Consider surfacing errors to admin users
- Handle specific exceptions rather than catching all
- Add recovery mechanisms for registration failures
public function register_single_store_custom_endpoint( string $option_name, array $option_value, array $old_options ) { if ( 'dokan_general' !== $option_name || ! isset( $old_options['custom_store_url'] ) || $old_options['custom_store_url'] === $option_value['custom_store_url'] ) { return; } try { icl_register_string( $this->wp_endpoints, $option_value['custom_store_url'], $option_value['custom_store_url'], false, wpml_get_default_language() ); - } catch ( Exception $e ) { - dokan_log( 'Dokan WPML - Error on registering vendor store URL endpoint: ' . $e->getMessage() ); + } catch ( InvalidArgumentException $e ) { + dokan_log( 'Dokan WPML - Invalid argument while registering vendor store URL endpoint: ' . $e->getMessage() ); + add_action( 'admin_notices', function() use ($e) { + printf( + '<div class="notice notice-error"><p>%s</p></div>', + esc_html__( 'Failed to register store URL for translation: ' . $e->getMessage(), 'dokan-wpml' ) + ); + }); + } catch ( RuntimeException $e ) { + dokan_log( 'Dokan WPML - Runtime error while registering vendor store URL endpoint: ' . $e->getMessage() ); + add_action( 'admin_notices', function() use ($e) { + printf( + '<div class="notice notice-error"><p>%s</p></div>', + esc_html__( 'Failed to register store URL for translation: ' . $e->getMessage(), 'dokan-wpml' ) + ); + }); } }
1376-1415: Consider optimizing rewrite rules update.The implementation is solid but could benefit from performance optimization:
flush_rewrite_rules()is an expensive operation- Consider batching rewrite rule updates
public function handle_translated_single_store_endpoint_rewrite( string $url_slug ) { global $sitepress; if ( ! $sitepress ) { return; } $languages = $sitepress->get_active_languages(); $slug_list = []; foreach ( $languages as $code => $language ) { $translated_slug = $this->translate_endpoint( $url_slug, $code ); if ( $translated_slug === $url_slug ) { continue; } $slug_list[] = $translated_slug; } if ( empty( $slug_list ) ) { return; } + // Batch process rewrite rules + $needs_flush = false; foreach ( $slug_list as $translated_slug ) { $this->add_rewrite_rules( $translated_slug, $url_slug ); + $needs_flush = true; } - flush_rewrite_rules(); + if ( $needs_flush ) { + flush_rewrite_rules(); + } }
1417-1437: Add documentation for rewrite rule patterns.The implementation is correct but would benefit from documentation explaining the URL patterns and their purposes.
public function add_rewrite_rules( string $regex_slug, string $query_slug, string $after = 'top' ) { + // Basic store URL pattern: example.com/store/vendor-name/ add_rewrite_rule( $regex_slug . '/([^/]+)/?$', 'index.php?' . $query_slug . '=$matches[1]', $after ); + + // Store pagination pattern: example.com/store/vendor-name/page/2/ add_rewrite_rule( $regex_slug . '/([^/]+)/page/?([0-9]{1,})/?$', 'index.php?' . $query_slug . '=$matches[1]&paged=$matches[2]', $after ); + // Store section pattern: example.com/store/vendor-name/section/123/ add_rewrite_rule( $regex_slug . '/([^/]+)/section/?([0-9]{1,})/?$', 'index.php?' . $query_slug . '=$matches[1]&term=$matches[2]&term_section=true', $after ); + + // Store section pagination pattern: example.com/store/vendor-name/section/123/page/2/ add_rewrite_rule( $regex_slug . '/([^/]+)/section/?([0-9]{1,})/page/?([0-9]{1,})/?$', 'index.php?' . $query_slug . '=$matches[1]&term=$matches[2]&paged=$matches[3]&term_section=true', $after ); + // Store terms & conditions pattern: example.com/store/vendor-name/toc/ add_rewrite_rule( $regex_slug . '/([^/]+)/toc?$', 'index.php?' . $query_slug . '=$matches[1]&toc=true', $after ); + + // Store terms & conditions pagination pattern: example.com/store/vendor-name/toc/page/2/ add_rewrite_rule( $regex_slug . '/([^/]+)/toc/page/?([0-9]{1,})/?$', 'index.php?' . $query_slug . '=$matches[1]&paged=$matches[2]&toc=true', $after ); }
1464-1484: Consider adding caching for database queries.While the implementation is secure and correct, frequent translation updates could benefit from caching the string ID mappings.
public function get_original_string_id( int $translated_string_id ): ?int { global $wpdb; + $cache_key = 'dokan_wpml_original_string_' . $translated_string_id; + $cached_value = wp_cache_get( $cache_key ); + + if ( false !== $cached_value ) { + return $cached_value; + } - return $wpdb->get_var( + $original_id = $wpdb->get_var( $wpdb->prepare( "SELECT string_id FROM {$wpdb->prefix}icl_string_translations WHERE id = %d", $translated_string_id ) ); + + wp_cache_set( $cache_key, $original_id, '', HOUR_IN_SECONDS ); + + return $original_id; }
1486-1506: Add caching for string value lookups.Similar to the previous method, implement caching to optimize frequent string value lookups.
public function get_original_string_value( int $string_id ): ?string { global $wpdb; + $cache_key = 'dokan_wpml_string_value_' . $string_id; + $cached_value = wp_cache_get( $cache_key ); + + if ( false !== $cached_value ) { + return $cached_value; + } - return $wpdb->get_var( + $string_value = $wpdb->get_var( $wpdb->prepare( "SELECT value FROM {$wpdb->prefix}icl_strings WHERE id = %d", $string_id ) ); + + wp_cache_set( $cache_key, $string_value, '', HOUR_IN_SECONDS ); + + return $string_value; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dokan-wpml.php(2 hunks)
🔇 Additional comments (2)
dokan-wpml.php (2)
1326-1345: LGTM! Clean implementation of initial endpoint registration.The method properly handles duplicate checks and follows WordPress coding standards.
1439-1462: LGTM! Well-structured translation update handler.The method follows a clean chain of responsibility pattern and properly validates data at each step.
Translation support added for Dokan single store translation support.
How to test the changes in this Pull Request:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements