Add CSV download through AJAX - #449
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a signed AJAX download endpoint for CSV exports and returns its URL in export responses; admin JS prefers that URL and trims whitespace before use. Also expands PHP-CS-Fixer Finder exclusions to ignore Changes
Sequence Diagram(s)sequenceDiagram
participant U as Admin User
participant UI as Admin UI (JS)
participant AE as AJAX Export Endpoint
participant AD as AJAX Download Endpoint
participant FS as Filesystem
U->>UI: Start CSV export
UI->>AE: ajax_csv_export(state)
AE-->>UI: Progress updates
AE-->>UI: Final response with download_url (or fileurl)
UI-->>U: Show "Download" link (uses trimmed download_url/fileurl)
U->>UI: Click download link
UI->>AD: ajax_csv_download(state + nonce)
AD->>FS: Locate and read file
FS-->>AD: File contents
AD-->>UI: Stream file (headers + bytes)
UI-->>U: Browser downloads CSV
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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
🧹 Nitpick comments (3)
includes/admin/csv-export.php (3)
86-92: Consider updating the version placeholder in the docblock.The
@sincetag currently showsx.xwhich should be replaced with the actual version number before merging.
140-147: Consider using readfile() or chunked reading for large files.While using
$wp_filesystem->get_contents()works, it loads the entire file into memory. For large CSV exports, this could cause memory issues. Consider using readfile() or chunked reading for better memory efficiency.-$file_content = $wp_filesystem->get_contents( $file_path ); - -if ( ! $file_content ) { - wp_die( esc_html__( 'Could not read export file.', 'business-directory-plugin' ) ); -} - -echo $file_content; +// Use readfile for better memory efficiency with large files +$result = @readfile( $file_path ); + +if ( false === $result ) { + wp_die( esc_html__( 'Could not read export file.', 'business-directory-plugin' ) ); +}Alternatively, for even better control and compatibility with WordPress Filesystem:
-$file_content = $wp_filesystem->get_contents( $file_path ); - -if ( ! $file_content ) { - wp_die( esc_html__( 'Could not read export file.', 'business-directory-plugin' ) ); -} - -echo $file_content; +// Read and output file in chunks for better memory efficiency +$handle = @fopen( $file_path, 'rb' ); + +if ( ! $handle ) { + wp_die( esc_html__( 'Could not read export file.', 'business-directory-plugin' ) ); +} + +while ( ! feof( $handle ) ) { + echo fread( $handle, 8192 ); // 8KB chunks + flush(); +} + +fclose( $handle );
155-162: Consider updating the version placeholder in the docblock.Similar to the earlier comment, the
@sincetag showsx.xwhich should be replaced with the actual version number.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
assets/js/admin-csv-import.min.jsis excluded by!**/*.min.js,!**/*.min.jsassets/js/admin-export.min.jsis excluded by!**/*.min.js,!**/*.min.jsassets/js/onboarding-wizard.min.jsis excluded by!**/*.min.js,!**/*.min.js
📒 Files selected for processing (3)
.php-cs-fixer.php(1 hunks)assets/js/admin-export.js(1 hunks)includes/admin/csv-export.php(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
includes/admin/csv-export.php (3)
includes/admin/helpers/csv/class-csv-exporter.php (3)
get_file_url(289-298)from_state(151-174)get_file_path(281-287)includes/helpers/class-app.php (2)
WPBDP_App_Helper(9-573)permission_check(212-231)includes/utils.php (1)
wpbdp_get_var(570-591)
🔇 Additional comments (9)
.php-cs-fixer.php (1)
5-6: LGTM!The addition of
node_modulesto the exclusion list is appropriate and follows standard practice for PHP projects that use npm packages.includes/admin/csv-export.php (7)
15-15: LGTM! Well-implemented AJAX download handler registration.Correctly registers the new AJAX action handler for CSV downloads, following WordPress conventions.
70-79: Good implementation of backward-compatible download URL.The response structure correctly includes both the new
download_urlfield and maintains the existingfileurlfield for backward compatibility. The conditional generation of the download URL only when the export is done is appropriate.
93-108: Solid permission and validation checks.The method properly validates permissions, verifies the nonce, and validates the state parameter before processing. The error messages are clear and properly escaped.
114-119: Good use of WordPress Filesystem API.Properly initializes the WordPress Filesystem API when needed, which is the recommended approach for file operations in WordPress plugins.
128-134: Excellent header implementation to force file download.The headers correctly:
- Override problematic
text/csvheaders withapplication/octet-stream- Force download with
Content-Disposition: attachment- Include file size for download progress
- Prevent caching to ensure fresh downloads
This directly addresses the PR's objective of forcing CSV downloads instead of displaying in the browser.
136-138: Proper output buffer cleanup.The loop ensures all output buffers are cleared before sending file content, preventing any unwanted output from corrupting the download.
163-172: Well-structured download URL generation.The method correctly creates a signed admin-ajax URL with all necessary parameters for secure file download. The use of
wp_create_nonce()ensures the download link is secure and can't be easily forged.assets/js/admin-export.js (1)
86-89: Clean implementation with proper fallback support.The code elegantly handles both the new AJAX download endpoint and maintains backward compatibility by falling back to the direct file URL when
download_urlis not available. This ensures the feature works correctly with both old and new server responses.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
includes/admin/csv-export.php (2)
86-92: Update the @SInCE version tag.The
@since x.xplaceholder should be replaced with the actual version number for this release.- * @since x.x + * @since 6.4.3
162-169: Update the @SInCE version tag.The
@since x.xplaceholder should be replaced with the actual version number for this release.- * @since x.x + * @since 6.4.3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/admin/csv-export.php(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
includes/admin/csv-export.php (4)
assets/js/onboarding-wizard/utils/url.js (1)
state(44-44)includes/admin/helpers/csv/class-csv-exporter.php (5)
get_file_url(289-298)WPBDP_CSVExporter(13-496)from_state(151-174)get_file_path(281-287)header(300-315)includes/helpers/class-app.php (1)
permission_check(212-231)includes/utils.php (1)
wpbdp_get_var(570-591)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PHPStan
🔇 Additional comments (10)
includes/admin/csv-export.php (10)
15-15: LGTM: AJAX handler registration follows WordPress conventions.The new AJAX action is properly registered using WordPress hooks with the expected naming pattern.
70-79: LGTM: Response structure enhanced with download URL.The response array has been properly restructured and includes the new
download_urlfield that will be used by the frontend JavaScript to trigger forced downloads.
93-96: LGTM: Proper security checks implemented.The function correctly implements WordPress security best practices with permission checking and nonce verification.
97-107: LGTM: State validation with appropriate error handling.The state parameter validation is thorough and uses WordPress's
wp_die()function for proper error termination in AJAX contexts.
109-116: LGTM: Export reconstruction and file validation.The code properly reconstructs the export object from state and validates file existence before proceeding with the download.
118-127: LGTM: Headers correctly configured to force download.The headers are well-configured to solve the original problem:
application/octet-streamoverrides hosting provider'stext/csvheadersContent-Disposition: attachmentforces download instead of browser display- Cache control headers prevent caching issues
129-131: LGTM: Output buffer cleanup before file streaming.Properly clears all output buffers to prevent interference with the file download stream.
134-151: LGTM: Efficient file streaming implementation.The chunked file reading approach is memory-efficient and includes proper error handling. The 8KB chunk size is appropriate for balancing memory usage and performance.
153-160: LGTM: Proper resource cleanup and error handling.File handle is properly closed and exceptions are handled with
wp_die()for consistent error reporting.
171-180: LGTM: URL generation follows WordPress conventions.The download URL is properly constructed using
add_query_arg()and includes all necessary parameters with proper nonce generation.
shervElmi
left a comment
shervElmi
left a comment
There was a problem hiding this comment.
LGTM!
Thanks @sorinmarta.
|
Thanks @shervElmi! |
2 participants
fixes https://github.com/Strategy11/business-directory-premium/issues/317
This PR forces download of CSV files on export by forcing the right headers on the download link. This is needed because some hosting providers add the
Content-Type: text/csvto the files so the browser open the contents as text.We do this without using the WordPress filesystem because we will have large export files that need to be streamed and they should not be loaded in memory.