enhance: support for additional CPT's #1483
Conversation
WalkthroughThe changes introduce significant enhancements to the subscription management system across multiple files. Key modifications include the addition of a new meta field for handling custom post type options, improved logic for managing non-recurring subscriptions, and refined validation for form submissions. These updates aim to provide a more structured and flexible approach to subscription data handling and user interactions. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (19)
includes/Api/Subscription.php (1)
422-424: LGTM! Consider adding a comment for clarity.The addition of
additional_cpt_optionsis well-implemented and aligns with the PR objective to support additional CPTs. The value is properly sanitized before being used.Consider adding a brief comment explaining the purpose of
additional_cpt_optionsfor better code readability:+ // Store additional custom post type options for extended functionality update_post_meta( $id, 'additional_cpt_options', $additional_cpt_options );Also applies to: 499-499
includes/Ajax/Frontend_Form_Ajax.php (9)
Line range hint
47-93: Use ofstrlen()May Not Handle Multibyte Characters CorrectlyThe
strlen()function counts bytes rather than characters, which can lead to incorrect character counts for multibyte character sets like UTF-8. This may result in validation errors for users inputting non-ASCII characters. Consider usingmb_strlen()to accurately count the number of characters.Apply this diff to replace
strlen()withmb_strlen():if ( 'character' === $restriction_type && 'min' === $restriction_to ) { - if ( strlen( $current_data ) > 0 && strlen( $current_data ) < $restricted_num ) { + if ( mb_strlen( $current_data ) > 0 && mb_strlen( $current_data ) < $restricted_num ) { wpuf()->ajax->send_error( sprintf( __( 'Minimum %d character is required for %s', 'wp-user-frontend' ), $restricted_num, $label ) ); } } elseif ( 'character' === $restriction_type && 'max' === $restriction_to ) { - if ( strlen( $current_data ) > 0 && strlen( $current_data ) > $restricted_num ) { + if ( mb_strlen( $current_data ) > 0 && mb_strlen( $current_data ) > $restricted_num ) { wpuf()->ajax->send_error( sprintf( __( 'Maximum %d character is allowed for %s', 'wp-user-frontend' ), $restricted_num, $label ) ); } }
Line range hint
47-93:str_word_count()May Not Accurately Count Words in All LanguagesThe
str_word_count()function may not correctly count words in languages that use multibyte characters or non-Latin scripts. This could lead to inaccurate validation for such users. Consider using a more robust method or a library that supports multibyte string word counts.As an alternative, you might use
mb_strlen()in conjunction with regex patterns to count words accurately:$word_count = preg_match_all( '/\pL+/u', $current_data, $matches );
Line range hint
70-93: Error Messages Lack Proper PluralizationThe error messages do not account for singular and plural forms, which may cause grammatical inaccuracies. Utilize the
_n()function to handle pluralization in translations, providing a better user experience.Apply this diff to implement pluralization:
// For character count restrictions - __( 'Minimum %d character is required for %s', 'wp-user-frontend' ) + sprintf( _n( 'Minimum %d character is required for %s', 'Minimum %d characters are required for %s', $restricted_num, 'wp-user-frontend' ), $restricted_num, $label ) // Similarly for other messages - __( 'Maximum %d word is allowed for %s', 'wp-user-frontend' ) + sprintf( _n( 'Maximum %d word is allowed for %s', 'Maximum %d words are allowed for %s', $restricted_num, 'wp-user-frontend' ), $restricted_num, $label )
Line range hint
47-93: Refactor Content Restriction Logic to Reduce DuplicationThe content restriction checks for character and word counts contain repetitive code blocks for each condition. Refactoring this logic into a single function or using a mapping strategy can enhance readability and maintainability.
Consider creating a helper function to handle the restriction checks:
private function check_content_restriction( $restriction_type, $restriction_to, $restricted_num, $current_data, $label ) { // Implement combined logic here }
Line range hint
95-114: Ensure Multibyte Support in Shortcode Restriction ChecksWhen checking for restricted shortcodes, using
strpos()may not accurately detect shortcodes with multibyte characters. Consider usingmb_strpos()for multibyte support.Apply this diff to use
mb_strpos():if ( mb_strpos( $current_data, $search_for ) !== false ) { wpuf()->ajax->send_error( sprintf( __( 'Using %s as shortcode is restricted', 'wp-user-frontend' ), $shortcode ) ); }
Line range hint
550-565: Inconsistent Error Handling MethodThe code uses
echo json_encode()followed bydie()to return an error, which is inconsistent with the use ofwpuf()->ajax->send_error()elsewhere. This may lead to confusion and potential maintenance issues. Align the error handling mechanism for consistency.Apply this diff to standardize error handling:
- 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
550-580: Remove Commented-Out Code for CleanlinessThere are blocks of commented-out code which may clutter the source and hinder readability. It's good practice to remove such code if it's no longer needed, as version control systems keep track of code changes.
Apply this diff to remove unnecessary commented code:
- // $this->send_error( __( 'Invalid email address.', 'wp-user-frontend' ) ); - // wp_send_json( - // [ - // 'success' => false, - // 'error' => __( "You already have an account in our site. Please login to continue.\n\nClicking 'OK' will redirect you to the login page and you will lose the form data.\nClick 'Cancel' to stay at this page.", 'wp-user-frontend' ), - // 'type' => 'login', - // 'redirect_to' => wp_login_url( get_permalink( $page_id ) ), - // ] - // ); - // wpuf()->ajax->send_error( __( 'Invalid email address.', 'wp-user-frontend' ) );
Line range hint
600-648: Sanitize User Input When Registering New UsersWhen creating a new user account, ensure all user-provided data, such as
$username, is properly sanitized to prevent security issues like SQL injection or invalid usernames.Apply this diff to sanitize the username:
$errors = new WP_Error(); + $username = sanitize_user( $username ); do_action( 'register_post', $username, $guest_email, $errors );
Line range hint
744-773: Potential Undefined Index in Custom Field ReplacementIn the
prepare_mail_body()method, when replacing custom fields in the email content, there might be cases where$matchesdoes not contain expected indices, potentially causing an undefined index notice. Add checks to ensure indices exist before using them.Apply this diff to prevent undefined index errors:
- [ $search, $replace ] = $matches; + if ( isset( $matches[0], $matches[1] ) ) { + $search = $matches[0]; + $replace = $matches[1]; + } else { + $search = []; + $replace = []; + }class/subscription.php (2)
296-296: Inconsistent meta key naming for'additional_cpt_options'The meta key
'additional_cpt_options'is not prefixed with an underscore, unlike other meta keys in this method (e.g.,'_billing_amount'). For consistency and to follow WordPress conventions, consider prefixing the meta key with an underscore.
Line range hint
597-612: Incorrect usage of$userdataobject ininsert_free_pack_subscribersThe method
insert_free_pack_subscribersis accessing properties of the$userdataobject incorrectly. User ID should be accessed via$userdata->ID, and display name via$userdata->display_name. Currently, the code uses$userdata->idand$userdata->user->data->display_name, which may not retrieve the correct information.Apply this diff to correct the property accesses:
-public function insert_free_pack_subscribers( $pack_id, $userdata ) { +public function insert_free_pack_subscribers( $pack_id, $user ) { global $wpdb; $subscription = wpuf()->subscription->get_subscription( $pack_id ); - if ( $userdata->id && $subscription ) { - $user_sub = self::get_user_pack( $userdata->id ); + if ( $user->ID && $subscription ) { + $user_sub = self::get_user_pack( $user->ID ); $post_expiration_time = wpuf_date2mysql( $user_sub['expire'] ); $table_data = [ - 'user_id' => $userdata->id, - 'name' => $userdata->user->data->display_name, + 'user_id' => $user->ID, + 'name' => $user->display_name, 'subscribtion_id' => $pack_id, 'subscribtion_status' => 'free', 'gateway' => 'free', 'transaction_id' => 'free', 'starts_from' => gmdate( 'd-m-Y' ), 'expire' => empty( $post_expiration_time ) ? 'recurring' : $post_expiration_time, ]; $wpdb->insert( $wpdb->prefix . 'wpuf_subscribers', $table_data ); } }includes/Admin/Admin_Subscription.php (7)
80-80: Undefined Index in ArrayThere is an extra comma at the end of the array on line 80, which could lead to a
ParseErrorin PHP versions earlier than 7.3.Remove the trailing comma to ensure compatibility with older PHP versions:
- 'default' => '-1', + 'default' => '-1'
Line range hint
294-296: Inefficient Use ofintvalfor Amount ComparisonIn lines 294-296, you are using
intval( $amount ) == 0to check if the amount is zero. This might not handle string amounts correctly and could lead to unexpected results if$amountis not numeric.Consider using a strict comparison and ensuring that
$amountis properly sanitized and validated as a float:- if ( intval( $amount ) == 0 ) { + if ( floatval( $amount ) === 0.0 ) {Also, ensure
$amountis sanitized:$amount = floatval( get_post_meta( $post_ID, '_billing_amount', true ) );
Line range hint
653-653: Unnecessary Escaping FunctionOn line 653,
esc_attris used when outputting a URL withinadmin_url(), butadmin_url()already returns a safe URL, and echoing it withinesc_attr()inside an HTML attribute is appropriate.For clarity and correctness, use
esc_urlinstead when outputting URLs:- echo esc_attr( admin_url( 'edit.php?post_type=wpuf_subscription' ) ) + echo esc_url( admin_url( 'edit.php?post_type=wpuf_subscription' ) )
Line range hint
670-670: Avoid Using Deprecated Constructor SyntaxIn line 670, you have an instance where the class name is used as a constructor. This syntax is deprecated as of PHP 7.0.
Ensure that all constructors use the
__constructmethod instead of the class name. Review the class definitions and update accordingly.
Line range hint
727-731: Mismatch in Translation FunctionIn lines 727-731, the
printffunction is used without wrapping the placeholder text with a translation function, which can cause issues with localization.Wrap the placeholder text within the translation function:
-__( 'You may use: %1$s %2$s %3$s %4$s %5$s', 'wp-user-frontend' ), +printf( + __( 'You may use: %1$s %2$s %3$s %4$s %5$s', 'wp-user-frontend' ), + '{post_author}', + '{post_url}', + '{blogname}', + '{post_title}', + '{post_status}' )Ensure that the placeholders are correctly passed to
sprintf()orprintf().
Line range hint
1399-1401: Potential Variable CollisionIn lines 1399-1401, the variable
$fieldsis being redefined multiple times throughout the method. This can lead to confusion and potential errors.Consider renaming the variables or combining the arrays to prevent overwriting:
$fields = [ 'subscription_details' => array_merge( $overview_fields, $access_fields, $expiration_fields ), 'payment_settings' => $payment_fields, 'advanced_configuration' => array_merge( $content_limit_fields, $design_element_fields, $additional_options_fields ), ];
Line range hint
1561-1561: Hardcoded URL in Helper TextOn line 1561, there is a hardcoded URL within a translation function, which can cause issues if the URL needs to be updated or if it's different in other languages.
Use
sprintfto separate the URL from the translatable text:-__( 'Use the <a href="%s">classic UI</a>.', 'wp-user-frontend' ), admin_url( 'edit.php?post_type=wpuf_subscription' ) +sprintf( + __( 'Use the <a href="%s">classic UI</a>.', 'wp-user-frontend' ), + esc_url( admin_url( 'edit.php?post_type=wpuf_subscription' ) ) +)Also, ensure that
esc_urlis used to sanitize the URL.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
assets/js/subscriptions.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (9)
- assets/js/stores/subscription.js (1 hunks)
- class/subscription.php (2 hunks)
- includes/Admin/Admin_Subscription.php (1 hunks)
- includes/Admin/Forms/Form.php (0 hunks)
- includes/Admin/Subscription.php (2 hunks)
- includes/Ajax/Frontend_Form_Ajax.php (1 hunks)
- includes/Api/Subscription.php (2 hunks)
- includes/Traits/TaxableTrait.php (1 hunks)
- includes/User_Subscription.php (1 hunks)
💤 Files with no reviewable changes (1)
- includes/Admin/Forms/Form.php
🔇 Additional comments (3)
assets/js/stores/subscription.js (1)
Line range hint
1-458: Ensure alignment with PR objectives and conduct thorough testingThe changes in the
modifyCurrentSubscriptionmethod appear to be aimed at improving the handling of custom post type options, which aligns with the PR objective of enhancing support for additional CPTs. However, it's crucial to ensure that this change fully addresses the issue of new options not being recognized by the subscription module.To validate that the changes meet the PR objectives and don't introduce unintended side effects:
Test the integration with plugins that generate new additional options:
- Install a plugin that generates additional options.
- Verify that these new options are correctly recognized and processed by the subscription module.
- Ensure that existing additional options continue to function as expected.
Check for any impacts on the publish time input option mentioned in the linked issue Fix: publish time input option in the Date/Time field, is not working… #637:
- Verify that the publish time input in the Date/Time field is functioning correctly after these changes.
Run comprehensive unit and integration tests focusing on subscription data handling, especially for custom post types and meta values.
Please provide the results of these tests to ensure the changes fully address the PR objectives without introducing new issues.
includes/Api/Subscription.php (1)
422-424: Verify usage of the newadditional_cpt_optionsfieldWhile the implementation looks good, it's important to ensure that other parts of the system can handle this new meta field.
Let's check if there are any other places in the codebase that might need to be updated to work with this new field:
Also applies to: 499-499
✅ Verification successful
Further verify the integration of
additional_cpt_optionswithwpuf_subscriptionThe initial search did not reveal any
get_post_metacalls related towpuf_subscription. To ensure comprehensive coverage, please run the following additional checks:
Further investigate interactions between
wpuf_subscriptionandadditional_cpt_optionsThe previous search did not reveal any interactions between
wpuf_subscriptionandadditional_cpt_options. To ensure comprehensive verification, please run the following additional checks:
Usage of
additional_cpt_optionsis properly verifiedThe
additional_cpt_optionsfield is consistently handled across the codebase without interfering with existingwpuf_subscriptionfunctionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of 'additional_cpt_options' in PHP files rg --type php 'additional_cpt_options' # Search for get_post_meta calls that might need to include this new field rg --type php 'get_post_meta.*wpuf_subscription'Length of output: 1532
Script:
#!/bin/bash # Search for any interactions between 'wpuf_subscription' and 'additional_cpt_options' rg --type php 'wpuf_subscription.*additional_cpt_options'Length of output: 59
Script:
#!/bin/bash # Search for any usage of 'wpuf_subscription' in PHP files rg --type php 'wpuf_subscription' # Search for any usage of 'additional_cpt_options' in PHP files rg --type php 'additional_cpt_options' # Search for any functions or methods that handle both 'wpuf_subscription' and 'additional_cpt_options' rg --type php 'wpuf_subscription.*additional_cpt_options|additional_cpt_options.*wpuf_subscription'Length of output: 15508
class/subscription.php (1)
Line range hint
597-612: Verify usage ofinsert_free_pack_subscribersmethodPlease ensure that the newly added method
insert_free_pack_subscribersis being called appropriately within the codebase so that free pack subscribers are properly inserted into the subscribers list.Run the following script to check for calls to
insert_free_pack_subscribers:✅ Verification successful
Verified usage of
insert_free_pack_subscribersmethodThe method
insert_free_pack_subscribersis properly called inincludes/User_Subscription.phpas expected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for calls to `insert_free_pack_subscribers` method in PHP files. # Test: Search for method calls to `insert_free_pack_subscribers`. Expect at least one occurrence. rg --type php '->insert_free_pack_subscribers\s*\('Length of output: 458
Script:
#!/bin/bash # Description: Search for calls to `insert_free_pack_subscribers` method in PHP files using the correct rg syntax. # Corrected Test: Search for method calls to `insert_free_pack_subscribers`. Expect at least one occurrence. rg --type php -e '->insert_free_pack_subscribers\s*\('Length of output: 180
assets/js/stores/subscription.js
Outdated
| if ( typeof this.currentSubscription.meta_value[key] === 'string' ) { | ||
| this.currentSubscription.meta_value[key] = {}; | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Conversion of meta_value[key] from String to Object Affects Multiple Components
The verification indicates that meta_value is utilized in various parts of the codebase. Changing meta_value[key] from a string to an object in the modifyCurrentSubscription method may lead to inconsistencies and unexpected behaviors in other components that expect it to be a string.
Potential Issues:
- Data Inconsistency: Other parts of the application may assume
meta_value[key]is a string, leading to type errors or unexpected behavior. - Data Loss: Converting a string to an empty object discards the original value, which could result in loss of important data.
- Lack of Error Handling: Without logging or error reporting, debugging issues related to this change may become challenging.
Recommendations:
- Preserve Original Data:
if (typeof this.currentSubscription.meta_value[key] === 'string') { const originalValue = this.currentSubscription.meta_value[key]; console.warn(`Converting meta_value[${key}] from string to object. Original value: ${originalValue}`); this.currentSubscription.meta_value[key] = { originalValue }; }
- Audit Dependent Code: Review all instances where
meta_valueis used to ensure they handle the new object structure appropriately. - Implement Comprehensive Logging: Add logging mechanisms to track when and why conversions occur, aiding in future debugging efforts.
- Investigate Root Cause: Determine why
meta_value[key]is a string in the first place and address any underlying data handling inconsistencies.
🔗 Analysis chain
Ensure data integrity when converting meta_value[key] to an object
The added type check and conversion for meta_value[key] prevents errors when assigning properties to a string. However, there are some potential issues to consider:
- This change might alter existing data structures, affecting parts of the application that expect
meta_value[key]to be a string. - Converting to an empty object discards any existing string value, which could lead to data loss.
Consider the following improvements:
- Add error logging when this conversion occurs to aid in debugging:
if (typeof this.currentSubscription.meta_value[key] === 'string') {
console.warn(`Converting meta_value[${key}] from string to object. Original value: ${this.currentSubscription.meta_value[key]}`);
this.currentSubscription.meta_value[key] = {};
}- Instead of converting to an empty object, consider preserving the original string value:
if (typeof this.currentSubscription.meta_value[key] === 'string') {
const originalValue = this.currentSubscription.meta_value[key];
this.currentSubscription.meta_value[key] = { originalValue };
}- Investigate why
meta_value[key]might be a string and address the root cause if possible.
To ensure this change doesn't introduce regressions, please run the following script to check for other occurrences of meta_value usage:
This will help identify other parts of the code that might be affected by this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of meta_value in the codebase
rg --type js "meta_value" -C 3
Length of output: 11996
includes/User_Subscription.php
Outdated
| $additional_cpt_options = ! empty( $subscription->meta_value['additional_cpt_options'] ) ? $subscription->meta_value['additional_cpt_options'] : []; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Null Coalescing Operator for Default Value Assignment
For improved readability and cleaner code, consider using the null coalescing operator ?? instead of the ! empty() check with a ternary operator.
Apply this diff:
-$additional_cpt_options = ! empty( $subscription->meta_value['additional_cpt_options'] ) ? $subscription->meta_value['additional_cpt_options'] : [];
+$additional_cpt_options = $subscription->meta_value['additional_cpt_options'] ?? [];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $additional_cpt_options = ! empty( $subscription->meta_value['additional_cpt_options'] ) ? $subscription->meta_value['additional_cpt_options'] : []; | |
| $additional_cpt_options = $subscription->meta_value['additional_cpt_options'] ?? []; |
includes/Traits/TaxableTrait.php
Outdated
| @@ -384,7 +384,7 @@ function wpuf_tax_rate_country_state( $country, $state ) { | |||
| continue; | |||
| } | |||
| if ( $rate['state'] === $state && $rate['country'] === $country ) { | |||
| $tax_amount = $rate['rate']; | |||
| $tax_amount = ! empty( $rate['rate'] ) ? $rate['rate'] : 0; | |||
There was a problem hiding this comment.
Correct the check for 'rate' to accurately handle zero values
Using ! empty( $rate['rate'] ) may inadvertently treat a valid zero tax rate (0 or '0') as empty, causing $tax_amount to default to 0 even when a specific zero rate is intended. To ensure that zero rates are correctly assigned, consider using isset( $rate['rate'] ) instead.
Apply this diff to fix the condition:
- $tax_amount = ! empty( $rate['rate'] ) ? $rate['rate'] : 0;
+ $tax_amount = isset( $rate['rate'] ) ? $rate['rate'] : 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $tax_amount = ! empty( $rate['rate'] ) ? $rate['rate'] : 0; | |
| $tax_amount = isset( $rate['rate'] ) ? $rate['rate'] : 0; |
includes/Ajax/Frontend_Form_Ajax.php
Outdated
| /** | ||
| * @var array | ||
| */ | ||
| private $form_fields; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Proper Documentation for New Property
The new private property $form_fields lacks a descriptive docblock explaining its purpose. Consider adding a meaningful description to enhance code readability and maintainability.
Apply this diff to add documentation:
/**
- * @var array
+ * An array of form fields retrieved from the form configuration.
+ *
+ * @var array
*/
private $form_fields;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * @var array | |
| */ | |
| private $form_fields; | |
| /** | |
| * An array of form fields retrieved from the form configuration. | |
| * | |
| * @var array | |
| */ | |
| private $form_fields; |
| @@ -418,6 +419,7 @@ public function save_form_meta( $subscription_id, $post ) { | |||
| update_post_meta( $subscription_id, '_trial_duration', $trial_duration ); | |||
| update_post_meta( $subscription_id, '_trial_duration_type', $trial_duration_type ); | |||
| update_post_meta( $subscription_id, '_post_type_name', array_map( 'sanitize_text_field', $post_data['post_type_name'] ) ); | |||
| update_post_meta( $subscription_id, 'additional_cpt_options', array_map( 'sanitize_text_field', $post_data['additional_cpt_options'] ) ); | |||
There was a problem hiding this comment.
Ensure 'additional_cpt_options' exists in $post_data before accessing it
To prevent potential undefined index notices, please check if 'additional_cpt_options' exists in $post_data before accessing it.
Apply this diff to add the necessary check:
+if ( isset( $post_data['additional_cpt_options'] ) ) {
update_post_meta( $subscription_id, 'additional_cpt_options', array_map( 'sanitize_text_field', $post_data['additional_cpt_options'] ) );
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| update_post_meta( $subscription_id, 'additional_cpt_options', array_map( 'sanitize_text_field', $post_data['additional_cpt_options'] ) ); | |
| if ( isset( $post_data['additional_cpt_options'] ) ) { | |
| update_post_meta( $subscription_id, 'additional_cpt_options', array_map( 'sanitize_text_field', $post_data['additional_cpt_options'] ) ); | |
| } |
| @@ -304,6 +304,7 @@ public static function get_subscription_meta( $subscription_id, $pack_post = nul | |||
| $meta['_trial_duration_type'] = $meta['trial_duration_type']; | |||
| $meta['post_type_name'] = get_post_meta( $subscription_id, '_post_type_name', true ); | |||
| $meta['_post_type_name'] = $meta['post_type_name']; | |||
| $meta['additional_cpt_options'] = get_post_meta( $subscription_id, 'additional_cpt_options', true ); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use consistent meta key naming with underscore prefix
In the existing code, meta keys are stored with an underscore prefix (e.g., _billing_amount, _expiration_number). For consistency and to prevent unintended exposure via the WordPress API, consider prefixing 'additional_cpt_options' with an underscore when retrieving the meta value.
Apply this diff to adjust the meta key:
-$meta['additional_cpt_options'] = get_post_meta( $subscription_id, 'additional_cpt_options', true );
+$meta['_additional_cpt_options'] = get_post_meta( $subscription_id, '_additional_cpt_options', true );
+$meta['additional_cpt_options'] = $meta['_additional_cpt_options'];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $meta['additional_cpt_options'] = get_post_meta( $subscription_id, 'additional_cpt_options', true ); | |
| $meta['_additional_cpt_options'] = get_post_meta( $subscription_id, '_additional_cpt_options', true ); | |
| $meta['additional_cpt_options'] = $meta['_additional_cpt_options']; |
| @@ -431,6 +432,7 @@ | |||
| update_post_meta( $subscription_id, '_trial_duration', $trial_duration ); | |||
| update_post_meta( $subscription_id, '_trial_duration_type', $trial_duration_type ); | |||
| update_post_meta( $subscription_id, '_post_type_name', array_map( 'sanitize_text_field', $post_data['post_type_name'] ) ); | |||
| update_post_meta( $subscription_id, 'additional_cpt_options', array_map( 'sanitize_text_field', $post_data['additional_cpt_options'] ) ); | |||
There was a problem hiding this comment.
Add check for 'additional_cpt_options' existence before updating meta
To prevent undefined index notices, ensure that 'additional_cpt_options' exists in $post_data before using it. This check prevents potential PHP warnings if the key is not set. Additionally, consider prefixing the meta key with an underscore for consistency.
Apply this diff to add the necessary check and adjust the meta key:
+if ( ! empty( $post_data['additional_cpt_options'] ) && is_array( $post_data['additional_cpt_options'] ) ) {
+ $sanitized_options = array_map( 'sanitize_text_field', $post_data['additional_cpt_options'] );
+ update_post_meta( $subscription_id, '_additional_cpt_options', $sanitized_options );
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| update_post_meta( $subscription_id, 'additional_cpt_options', array_map( 'sanitize_text_field', $post_data['additional_cpt_options'] ) ); | |
| if ( ! empty( $post_data['additional_cpt_options'] ) && is_array( $post_data['additional_cpt_options'] ) ) { | |
| $sanitized_options = array_map( 'sanitize_text_field', $post_data['additional_cpt_options'] ); | |
| update_post_meta( $subscription_id, '_additional_cpt_options', $sanitized_options ); | |
| } |
| 'id' => $key, | ||
| 'name' => $key, | ||
| 'db_key' => 'additional_cpt_options', | ||
| 'db_type' => 'meta_serialized', | ||
| 'serialize_key' => $key, |
There was a problem hiding this comment.
Improper Use of Meta Keys and Serialization
The array keys 'db_key' and 'serialize_key' in lines 71 and 73 might cause confusion. You're setting 'db_key' to 'additional_cpt_options' and 'serialize_key' to $key, which could lead to inconsistent data storage and retrieval, especially when handling serialized meta values.
Consider refactoring to ensure that each custom post type option has a unique and coherent meta key. For example:
- 'db_key' => 'additional_cpt_options',
- 'db_type' => 'meta_serialized',
- 'serialize_key' => $key,
+ 'db_key' => 'additional_cpt_options_' . $key,
+ 'db_type' => 'meta',
+ 'serialize_key' => null,This change assigns a unique meta key for each post type option and avoids complex serialization.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'id' => $key, | |
| 'name' => $key, | |
| 'db_key' => 'additional_cpt_options', | |
| 'db_type' => 'meta_serialized', | |
| 'serialize_key' => $key, | |
| 'id' => $key, | |
| 'name' => $key, | |
| 'db_key' => 'additional_cpt_options_' . $key, | |
| 'db_type' => 'meta', | |
| 'serialize_key' => null, |
| 'label' => sprintf( 'Number of %s', esc_html( $post_type_object->label ) ), | ||
| 'tooltip' => sprintf( |
There was a problem hiding this comment.
Internationalization (i18n) Issue in String Formatting
The usage of sprintf with plain strings on lines 75 and 76 isn't prepared for translation. Strings should be wrapped with __() or _e() before being passed to sprintf for proper localization.
Update the code to wrap the strings with translation functions:
- 'label' => sprintf( 'Number of %s', esc_html( $post_type_object->label ) ),
- 'tooltip' => sprintf(
- 'Set the maximum number of %s users can create within their subscription period. Enter -1 for unlimited',
+ 'label' => sprintf( __( 'Number of %s', 'wp-user-frontend' ), esc_html( $post_type_object->label ) ),
+ 'tooltip' => sprintf(
+ __( 'Set the maximum number of %s users can create within their subscription period. Enter -1 for unlimited', 'wp-user-frontend' ),Committable suggestion was skipped due to low confidence.
Rubaiyat-E-Mohammad
added
bug
needs: dev review
…for_additional_cpts
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)
Line range hint
59-87: Usemb_strlen()for Multibyte Character SupportThe use of
strlen()may not correctly handle multibyte characters, leading to incorrect character counts for non-ASCII input. Consider usingmb_strlen()to accurately count the number of characters in$current_data.Apply this diff to replace
strlen()withmb_strlen():- if ( strlen( $current_data ) > 0 && strlen( $current_data ) < $restricted_num ) { + if ( mb_strlen( $current_data ) > 0 && mb_strlen( $current_data ) < $restricted_num ) {Ensure that the
mbstringPHP extension is enabled on the server.
Line range hint
71-87: Improve Word Count Handling for Internationalization
str_word_count()may not accurately count words in languages with multibyte characters or different word delimiters. This could lead to incorrect validation results for some users.Consider using a more reliable method for word counting that supports multibyte characters:
$word_count = count( preg_split( '/\s+/u', $current_data, -1, PREG_SPLIT_NO_EMPTY ) );Replace
str_word_count( $current_data )with$word_countfor accurate word counts.
Line range hint
90-103: Usehas_shortcode()for Accurate Shortcode DetectionUsing
strpos()to detect shortcodes may lead to false positives if the shortcode name appears in the content but not as an actual shortcode. Thehas_shortcode()function provides a reliable way to check for shortcodes in content.Replace the shortcode detection logic with:
if ( ! empty( $current_data ) ) { foreach ( $protected_shortcodes as $shortcode ) { if ( has_shortcode( $current_data, $shortcode ) ) { wpuf()->ajax->send_error( sprintf( __( 'Using %s shortcode is restricted', 'wp-user-frontend' ), $shortcode ) ); } } }Ensure that this code runs in a context where
has_shortcode()is available.
Line range hint
536-544: Consistent Error Handling withwpuf()->ajax->send_error()Using
echo json_encode()followed bydie();is inconsistent with the rest of the codebase, which primarily useswpuf()->ajax->send_error()for error responses. This inconsistency can lead to unexpected behavior and maintainability issues.Replace the error handling code with:
wpuf()->ajax->send_error( __( 'Invalid email address.', 'wp-user-frontend' ) );Remove the
echoanddie();statements to maintain consistency.
Line range hint
536-544: Remove Commented-Out Code to Improve ReadabilityThere are several blocks of commented-out code within the
wpuf_get_post_user()method. Commented code can clutter the codebase and make it harder to maintain.Consider removing the commented-out code unless it's necessary for future development.
Line range hint
604-608: Simplify Value Concatenation Logic inprepare_mail_body()The logic used to concatenate values in the custom fields replacement section seems unnecessarily complex and may result in duplicated commas.
Review and simplify the concatenation logic to ensure accurate and clean replacement values. For example:
if ( is_array( $value ) ) { $original_value = implode( ', ', array_map( function ( $val ) { return wp_get_attachment_url( $val ) ?: $val; }, $value ) ); } else { $original_value = wp_get_attachment_url( $value ) ?: $value; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- includes/Admin/Forms/Form.php (0 hunks)
- includes/Ajax/Frontend_Form_Ajax.php (1 hunks)
💤 Files with no reviewable changes (1)
- includes/Admin/Forms/Form.php
🧰 Additional context used
🔇 Additional comments (3)
includes/Ajax/Frontend_Form_Ajax.php (3)
24-27: Good Documentation Added for$form_fieldsThe docblock for the new private property
$form_fieldsclearly explains its purpose, enhancing code readability and maintainability.
Line range hint
53-55: Proper Initialization of$this->form_fieldsStoring the form fields in the class property
$this->form_fieldsimproves code organization and allows for easier access throughout the class methods.
Line range hint
151-151: Verify PHP Version Compatibility for Array DestructuringThe use of array destructuring with the list syntax requires PHP 7.1 or higher. Ensure that the minimum PHP version requirement for this project is at least 7.1 to avoid compatibility issues.
Check the project's PHP version compatibility by reviewing the server configuration or project documentation.
fixes #637
Description: In the revamped Subscriptions Module of WP User Frontend, we previously handled a number of additional options originating from other plugins (e.g., the number of events for Event Calendar). However, after the recent update, while the previously supported options continue to work, new additional options generated from other plugins are not functioning as expected.
Current Behavior:
Expected Behavior:
Steps to Reproduce:
Proposed Enhancement:
Summary by CodeRabbit
Release Notes
New Features
additional_cpt_options, allowing for more flexible metadata handling.Bug Fixes
Improvements