Add CSV export batch size filter and optimize entries query when Pro is not installed#2204
Conversation
WalkthroughThe changes in the Changes
Sequence DiagramsequenceDiagram
participant Developer
participant FrmCSVExportHelper
participant WordPress Filter
participant Database
Developer->>WordPress Filter: Set frm_csv_export_batch_size
FrmCSVExportHelper->>WordPress Filter: Apply batch size filter
alt Custom Batch Size
WordPress Filter-->>FrmCSVExportHelper: Return custom batch size
else Default Batch Size
WordPress Filter-->>FrmCSVExportHelper: Return default (20)
end
FrmCSVExportHelper->>Database: Fetch entries in batches
Database-->>FrmCSVExportHelper: Return entries
FrmCSVExportHelper->>FrmCSVExportHelper: Process and export CSV
Possibly related PRs
📜 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: 1
🧹 Nitpick comments (1)
classes/helpers/FrmCSVExportHelper.php (1)
148-156: Clarify the@sincetag and add references to the filter.
It’s recommended to replace@since x.xwith the actual release version and include an inline reference to the newfrm_csv_export_batch_sizefilter usage in the documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/helpers/FrmCSVExportHelper.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (1)
classes/helpers/FrmCSVExportHelper.php (1)
159-160: Chunking large dataset exports in a loop is effective.
This approach avoids loading all entries into memory at once, enhancing performance and preventing potential memory exhaustion issues for large datasets.
Crabcyborg
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
classes/helpers/FrmCSVExportHelper.php (1)
148-160: Consider documenting the performance implications.The batch size implementation would benefit from additional documentation:
- Document the memory usage characteristics
- Specify recommended batch sizes for different scenarios
- Add examples of filter usage
Add PHPDoc for the filter:
/** * For a target form with 50k entries (Locations List, with 7 fields), batch sizes of 100 * were significantly faster and used less memory, so a filter was added. * + * Example usage: + * ```php + * add_filter('frm_csv_export_batch_size', function($batch_size, $form_id) { + * return ($form_id === 123) ? 500 : $batch_size; + * }, 10, 2); + * ``` + * + * @note Larger batch sizes improve performance but increase memory usage. + * Recommended ranges: + * - Small forms (<1k entries): 20-50 + * - Medium forms (1k-10k entries): 50-200 + * - Large forms (>10k entries): 200-1000 * * @since x.x * * @param int $batch_size * @param int $form_id */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/helpers/FrmCSVExportHelper.php(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (2)
classes/helpers/FrmCSVExportHelper.php (2)
418-434: LGTM! Well-structured query optimization.The conditional query logic is well implemented:
- Efficiently handles Pro vs non-Pro scenarios
- Includes clear comments explaining the optimization
- Uses appropriate ordering for both cases
148-158:⚠️ Potential issueAdd validation for the batch size filter value.
The batch size filter implementation needs validation to prevent potential issues with array_splice():
- Zero could cause an infinite loop
- Negative values could cause unexpected behavior
Apply this diff to ensure a valid batch size:
-$csv_export_batch_size = (int) apply_filters( 'frm_csv_export_batch_size', 20, self::$form_id ); +$csv_export_batch_size = max( + 1, + (int) apply_filters( 'frm_csv_export_batch_size', 20, self::$form_id ) +);Likely invalid or redundant comment.
1 participant
Related ticket https://secure.helpscout.net/conversation/2809125713/219105
The issue
The solution
The result
CSV Exports for the Locations List (50k+ entries, 7 fields) decreased from 55s to 46s, when Pro is not active.