fix: integrations not loading properly #1486
Conversation
WalkthroughThe changes introduce a new class Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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: 1
🧹 Outside diff range and nitpick comments (3)
includes/Integrations.php (2)
11-18: LGTM: Property declaration is clear and well-documented.The
$containerproperty is appropriately named and initialized. The docblock provides clear information about its purpose.Consider making the
$containerproperty protected and providing a getter method for better encapsulation:- public $container = []; + protected $container = []; + + public function get_container() { + return $this->container; + }This change would allow you to maintain control over how the container is accessed and modified externally.
20-36: LGTM: Constructor logic is sound, but could be more extensible.The constructor effectively initializes various integrations based on the presence of specific classes. The use of
class_exists()checks is appropriate for conditional loading.Consider refactoring the constructor to improve extensibility and maintainability:
- Create a protected method to handle integration registration:
protected function register_integration($class_name, $integration_class, $key) { if (class_exists($class_name)) { $this->container[$key] = new $integration_class(); } }
- Update the constructor to use this method:
public function __construct() { $this->register_integration('WeDevs_Dokan', Integrations\WPUF_Dokan_Integration::class, 'dokan'); $this->register_integration('WC_Vendors', Integrations\WPUF_WC_Vendors_Integration::class, 'wc_vendors'); $this->register_integration('WCMp', Integrations\WPUF_WCMp_Integration::class, 'wcmp'); $this->register_integration('ACF', Integrations\WPUF_ACF_Compatibility::class, 'acf'); }This refactoring would make it easier to add or modify integrations in the future and improve the overall maintainability of the class.
wpuf.php (1)
176-176: LGTM! Consider optimizing integration loading.The addition of the
Integrationsclass to the container is a good step towards fixing the integration loading issues mentioned in the PR objectives. This change will allow for centralized management of various plugin integrations.However, to optimize performance, consider the following suggestions:
- Implement lazy loading for integrations, only instantiating them when needed.
- Add conditional checks to ensure that the required plugins for each integration are active before loading them.
Would you like me to provide an example of how to implement these optimizations?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- includes/Integrations.php (1 hunks)
- wpuf.php (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
includes/Integrations.php (1)
1-10: LGTM: Class structure and namespace are well-defined.The
Integrationsclass is properly namespaced and follows PSR-1 naming conventions. The docblock provides a concise description of the class purpose.wpuf.php (1)
Line range hint
1-476: Summary of changes and potential impactThe addition of the
Integrationsclass to the plugin's container is a positive step towards addressing the integration loading issues mentioned in the PR objectives. This change lays the groundwork for a more centralized and potentially more efficient management of various plugin integrations (ACF, Dokan, WC Vendors, WCFM).Key points:
- The change is minimal but strategically important.
- It doesn't directly solve the loading issues but provides the necessary infrastructure.
- The effectiveness of this change will depend on how the
Integrationsclass is implemented and utilized throughout the plugin.Next steps:
- Implement the
Integrationsclass to handle the loading of various integrations.- Update other parts of the plugin to use this new
Integrationsinstance where appropriate.- Consider optimizing the integration loading process as suggested in previous comments.
Overall, this change is a step in the right direction for improving the plugin's integration management.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
includes/Integrations.php (2)
5-10: ReplaceWPUF_SINCEplaceholder with the actual version number.The
@sincetag in the class docblock contains a placeholderWPUF_SINCE. To maintain accurate documentation, please replace this with the actual version number in which this class was introduced.
38-53: LGTM: Magic method implementation is correct. Consider adding type hints.The
__getmagic method is well-implemented and documented. It correctly handles access to the integration instances stored in the$containerarray.As a minor improvement, consider adding type hints to the method signature and return type:
public function __get(string $prop): ?objectThis would provide better clarity about the expected input and output types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- includes/Integrations.php (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
includes/Integrations.php (2)
11-18: LGTM: Property declaration is well-documented and appropriate.The
$containerproperty is correctly declared as a public array and is well-documented. Its purpose of holding various class instances is clearly explained in the docblock.
1-54: Overall assessment: Good implementation with room for improvementThe
Integrationsclass provides a solid foundation for managing various plugin integrations. The main points for improvement are:
- Refactoring the constructor for better maintainability and flexibility.
- Adding type hints to method signatures for improved code clarity.
- Replacing the
WPUF_SINCEplaceholder with the actual version number in the class docblock.These changes will enhance the code's readability, maintainability, and robustness. Great job on implementing this integration management system!
includes/Integrations.php
Outdated
| public function __construct() { | ||
| if ( class_exists( 'WeDevs_Dokan' ) ) { | ||
| $this->container['dokan'] = new Integrations\WPUF_Dokan_Integration(); | ||
| } | ||
|
|
||
| if ( class_exists( 'WC_Vendors' ) ) { | ||
| $this->container['wc_vendors'] = new Integrations\WPUF_WC_Vendors_Integration(); | ||
| } | ||
|
|
||
| if ( class_exists( 'WCMp' ) ) { | ||
| $this->container['wcmp'] = new Integrations\WPUF_WCMp_Integration(); | ||
| } | ||
|
|
||
| if ( class_exists( 'ACF' ) ) { | ||
| $this->container['acf'] = new Integrations\WPUF_ACF_Compatibility(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring for improved maintainability and flexibility.
While the constructor logic is functional, there are opportunities for improvement:
-
Consider using an array or configuration file to define the mappings between external classes and their corresponding integration classes. This would make it easier to add or modify integrations in the future.
-
The integration class names are currently hardcoded. Consider using a naming convention or configuration that allows for more dynamic loading of integration classes.
-
You might want to add error handling or logging for cases where an integration class fails to initialize.
Here's a potential refactor:
private $integrations = [
'WeDevs_Dokan' => 'WPUF_Dokan_Integration',
'WC_Vendors' => 'WPUF_WC_Vendors_Integration',
'WCMp' => 'WPUF_WCMp_Integration',
'ACF' => 'WPUF_ACF_Compatibility',
];
public function __construct() {
foreach ($this->integrations as $externalClass => $integrationClass) {
if (class_exists($externalClass)) {
$fullClassName = __NAMESPACE__ . '\\Integrations\\' . $integrationClass;
try {
$this->container[strtolower($externalClass)] = new $fullClassName();
} catch (\Exception $e) {
// Log the error or handle it appropriately
}
}
}
}This approach would make it easier to add new integrations in the future and provides better error handling.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
fixed. please have a look now |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
includes/Ajax/Frontend_Form_Ajax.php (6)
22-25: Provide a descriptive comment for$form_fieldspropertyThe docblock for the
$form_fieldsproperty currently only specifies the type as@var array. Consider adding a brief description to explain what this property holds, enhancing code readability and maintainability.Apply this diff to add a description:
/** + * Holds the fields of the form for validation and processing. * @var array */ private $form_fields;
Line range hint
57-98: Refactor content restriction validation to reduce code duplicationThe current implementation of content restriction validation for 'character' and 'word' counts with 'min' and 'max' conditions contains duplicated code blocks. Refactoring can improve maintainability and readability.
Consider refactoring the validation logic into a single function or using a mapping approach. Here's an example of how you might simplify the code:
foreach ( $this->form_fields as $single_field ) { // Continue if no content restriction if ( empty( $single_field['content_restriction'] ) ) { continue; } $restricted_num = $single_field['content_restriction']; $restriction_to = ! empty( $single_field['restriction_to'] ) ? $single_field['restriction_to'] : 'min'; $restriction_type = ! empty( $single_field['restriction_type'] ) ? $single_field['restriction_type'] : 'word'; $current_data = ! empty( $_POST[ $single_field['name'] ] ) ? wp_unslash( $_POST[ $single_field['name'] ] ) : ''; $label = ! empty( $single_field['label'] ) ? $single_field['label'] : ''; // Sanitize data based on field type if ( 'word' === $restriction_type ) { $current_data_length = str_word_count( wp_strip_all_tags( $current_data ) ); $type_label = 'word'; } else { $current_data_length = strlen( wp_strip_all_tags( $current_data ) ); $type_label = 'character'; } if ( $current_data_length > 0 ) { if ( 'min' === $restriction_to && $current_data_length < $restricted_num ) { wpuf()->ajax->send_error( sprintf( __( 'Minimum %d %s is required for %s', 'wp-user-frontend' ), $restricted_num, $type_label, $label ) ); } elseif ( 'max' === $restriction_to && $current_data_length > $restricted_num ) { wpuf()->ajax->send_error( sprintf( __( 'Maximum %d %s is allowed for %s', 'wp-user-frontend' ), $restricted_num, $type_label, $label ) ); } } }
Line range hint
970-987: Usewpuf()->ajax->send_error()for consistent error handlingIn the
wpuf_get_post_user()method, when validating the guest email, the error response is sent usingecho json_encode()followed bydie();. For consistency and better error handling, consider usingwpuf()->ajax->send_error().Apply this diff to fix the error handling:
if ( ! is_email( $guest_email ) ) { - echo json_encode( - [ - 'success' => false, - 'error' => __( 'Invalid email address.', 'wp-user-frontend' ), - ] - ); - die(); + wpuf()->ajax->send_error( __( 'Invalid email address.', 'wp-user-frontend' ) ); }
Line range hint
725-727: Correct typographical error in confirmation messageThe message "We have sent you an confirmation email." contains a typographical error. It should read "We have sent you a confirmation email."
Apply this diff to correct the message in both occurrences:
$response['message'] = __( 'Thank you for posting on our site. We have sent you a confirmation email. Please check your inbox!', 'wp-user-frontend' );Also applies to: 745-747
Line range hint
1240-1251: Fix variable assignment in custom field placeholder handlingIn the
prepare_mail_body()method, the assignment[ $search, $replace ] = $matches;is incorrect. Thepreg_match_allfunction returns an array where$matches[0]contains the full matches and$matches[1]contains the captured groups. Assigning them correctly ensures placeholders are properly replaced.Apply this diff to fix the variable assignment:
preg_match_all( '/{custom_([\w-]*)\b}/', $content, $matches ); -[ $search, $replace ] = $matches; +$search = $matches[0]; +$replace = $matches[1];
Line range hint
1252-1278: Simplify custom field value concatenation logicThe current logic for concatenating custom field values is complex and might result in redundant commas and values. Consider simplifying the logic to handle multiple values more efficiently.
Here is a refactored version:
foreach ( $replace as $index => $meta_key ) { $value = get_post_meta( $post_id, $meta_key, false ); if ( is_array( $value ) && count( $value ) > 0 ) { $meta_values = []; foreach ( $value as $val ) { if ( is_array( $val ) ) { $val = implode( '; ', $val ); } if ( get_post_mime_type( (int) $val ) ) { $meta_values[] = wp_get_attachment_url( $val ); } else { $meta_values[] = $val; } } $original_value = implode( ', ', $meta_values ); } else { $original_value = ''; } $content = str_replace( $search[ $index ], $original_value, $content ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- includes/Ajax/Frontend_Form_Ajax.php (1 hunks)
- includes/Frontend.php (1 hunks)
- includes/Frontend_Render_Form.php (0 hunks)
- templates/dokan/new-post.php (1 hunks)
💤 Files with no reviewable changes (1)
- includes/Frontend_Render_Form.php
✅ Files skipped from review due to trivial changes (1)
- templates/dokan/new-post.php
🧰 Additional context used
🔇 Additional comments (1)
includes/Frontend.php (1)
183-183: LGTM, but verify the impact of this change.The simplification of the
dokan_is_seller_dashboardmethod looks good as it removes an unnecessary check on the 'posts' query variable. This change aligns with the PR objective of fixing integration loading issues, as it may allow the Dokan integration to load in more scenarios.However, please verify that this change doesn't introduce any unintended side effects, such as unnecessarily loading Dokan-related scripts and styles on non-dashboard pages.
To ensure this change doesn't have unintended consequences, please run the following verification:
✅ Verification successful
Change Verified
The removal of the
&& ! empty( $GLOBALS['wp']->query_vars['posts'] )condition in thedokan_is_seller_dashboardmethod is isolated to theincludes/Frontend.phpfile. No other usages of this condition were found in the codebase, and the change does not impact other parts of the application. Therefore, the modification does not introduce unintended side effects and can be safely approved.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any other usages of the removed condition # Search for any other instances where the 'posts' query variable is checked in relation to Dokan rg --type php "dokan.*GLOBALS\['wp'\]->query_vars\['posts'\]" # Search for any other methods or functions that might be affected by this change rg --type php "function.*dokan_is_seller_dashboard"Length of output: 290
This comment was marked as resolved.
This comment was marked as resolved.
|
related PR for WCFM |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- includes/Integrations.php (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
includes/Integrations.php (4)
1-25: LGTM: Class structure and properties are well-defined.The
Integrationsclass is properly namespaced and structured. The$containerproperty is well-documented, and the$integrationsproperty uses a clear mapping structure for external classes to their corresponding integration classes.
27-38: LGTM: Constructor implementation is robust and follows best practices.The constructor efficiently initializes integration instances based on the presence of external classes. It includes proper error handling and logging for failed initializations. The use of
__NAMESPACE__ensures correct class resolution.
40-55: LGTM: Magic getter method is well-implemented.The
__getmethod provides a convenient and safe way to access integration instances. It correctly handles both existing and non-existing properties, returning null for the latter.
1-56: LGTM: Overall class design is modular and maintainable.The
Integrationsclass effectively manages multiple integrations using a configuration array. Its design is flexible, allowing for easy addition of new integrations. The class follows the Single Responsibility Principle and provides a clean interface for accessing integrations.
| <?php | ||
|
|
||
| namespace WeDevs\Wpuf; | ||
|
|
||
| /** | ||
| * The integration class to handle all integrations with our plugin | ||
| * | ||
| * @since WPUF_SINCE | ||
| */ | ||
| class Integrations { | ||
| /** | ||
| * Holds various class instances | ||
| * | ||
| * @since WPUF_SINCE | ||
| * | ||
| * @var array | ||
| */ | ||
| public $container = []; | ||
|
|
||
| private $integrations = [ | ||
| 'WeDevs_Dokan' => 'WPUF_Dokan_Integration', | ||
| 'WC_Vendors' => 'WPUF_WC_Vendors_Integration', | ||
| 'WCMp' => 'WPUF_WCMp_Integration', | ||
| 'ACF' => 'WPUF_ACF_Compatibility', | ||
| ]; | ||
|
|
||
| public function __construct() { | ||
| foreach ( $this->integrations as $external_class => $integration_class ) { | ||
| if ( class_exists( $external_class ) ) { | ||
| $full_class_name = __NAMESPACE__ . '\\Integrations\\' . $integration_class; | ||
| try { | ||
| $this->container[ strtolower( $external_class ) ] = new $full_class_name(); | ||
| } catch ( \Exception $e ) { | ||
| \WP_User_Frontend::log( 'integration', print_r( $external_class . ' integration failed', true ) ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Magic getter to bypass referencing objects | ||
| * | ||
| * @since WPUF_SINCE | ||
| * | ||
| * @param string $prop | ||
| * | ||
| * @return null|object Class Instance | ||
| */ | ||
| public function __get( $prop ) { | ||
| if ( array_key_exists( $prop, $this->container ) ) { | ||
| return $this->container[ $prop ]; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding methods for enhanced flexibility and utility.
While the current implementation is solid, consider the following enhancements:
- Add methods to dynamically add or remove integrations at runtime.
- Implement a method to check if a specific integration is available.
These additions would further improve the flexibility and utility of the Integrations class.
Here's a potential implementation:
public function addIntegration($externalClass, $integrationClass) {
$this->integrations[$externalClass] = $integrationClass;
// Attempt to initialize the new integration if the external class exists
if (class_exists($externalClass)) {
$fullClassName = __NAMESPACE__ . '\\Integrations\\' . $integrationClass;
try {
$this->container[strtolower($externalClass)] = new $fullClassName();
} catch (\Exception $e) {
\WP_User_Frontend::log('integration', print_r($externalClass . ' integration failed', true));
}
}
}
public function removeIntegration($externalClass) {
unset($this->integrations[$externalClass]);
unset($this->container[strtolower($externalClass)]);
}
public function hasIntegration($externalClass) {
return isset($this->container[strtolower($externalClass)]);
}
Rat01047
added
QA Approved
|
@sapayth bhai, |
fixes #1487
ACF, Dokan, WC Vendors, WCFMintegrations wasn't loading properly with WPUF.need to test all the integrations. some video references on how the integration works:
Dokan
ACF
WC Vendors
WC Marketplace/ WCFM [related PR]
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Integrationsclass to manage various integrations dynamically based on available plugins.Dokan,WC Vendors,WCMp, andACF.Improvements
WP_User_Frontendclass to include an instance of the newIntegrationsclass for better integration management.dokan_is_seller_dashboardmethod for improved clarity.