Fix: Refactor lightbox script enqueuing to prevent DataTables conflicts #2451
Conversation
- Replace direct wp_print_scripts() with proper register/enqueue pattern - Use wp_add_inline_script/style instead of direct HTML output - Add validation to prevent double-enqueuing of scripts/styles - Deprecate print_scripts() in favor of add_inline_scripts_and_styles() - Improve security with proper escaping (esc_js, esc_attr) - Add error handling for JSON encoding and script registration - Enhance FancyBox gallery ID generation with sanitization This resolves DataTables rendering failures caused by improper script loading order and timing conflicts. The refactoring follows WordPress best practices and maintains full backward compatibility. Fixes #2450 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughTransitioned lightbox assets from direct printing to a register-then-enqueue lifecycle. Added register_scripts/register_styles hooks, introduced add_inline_scripts_and_styles for inline assets, deprecated print_scripts, and updated FancyBox provider and entry-link flow to enqueue assets with duplicate checks and WordPress inline APIs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant WP as WordPress Hooks
participant View as GravityView Render
participant LB as Lightbox Provider (Base)
participant FB as FancyBox Provider
WP->>LB: add_hooks()<br/>register_scripts()/register_styles()
Note over LB: Assets are registered only
View->>LB: filter_entry_link_output()
LB->>LB: enqueue_styles()/enqueue_scripts()<br/>[skip if already enqueued]
LB->>FB: add_inline_scripts_and_styles()<br/>[inline JS/CSS after enqueue]
Note over FB: Binds Fancybox & sets z-index/admin-bar CSS
View-->>User: Rendered links with data-fancybox attributes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 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: 2
🧹 Nitpick comments (2)
includes/extensions/lightbox/class-gravityview-lightbox-provider.php (2)
69-70: Register assets earlier in the queue.Registering on wp_enqueue_scripts at default priority (10) is fine, but setting a lower priority helps guarantee handles exist before any downstream inline additions or conditional enqueues run.
Apply:
- add_action( 'wp_enqueue_scripts', array( $this, 'register_scripts' ) ); - add_action( 'wp_enqueue_scripts', array( $this, 'register_styles' ) ); + add_action( 'wp_enqueue_scripts', array( $this, 'register_scripts' ), 1 ); + add_action( 'wp_enqueue_scripts', array( $this, 'register_styles' ), 1 );
229-237: Docblock wording nit.These methods “register” assets (not “enqueue”). Minor doc tweak will reduce confusion.
Apply:
- * Enqueue scripts for the lightbox + * Register scripts for the lightboxand
- * Enqueue styles for the lightbox + * Register styles for the lightbox
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
includes/extensions/lightbox/class-gravityview-lightbox-provider.php(5 hunks)includes/extensions/lightbox/fancybox/class-gravityview-lightbox-provider-fancybox.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
includes/extensions/lightbox/fancybox/class-gravityview-lightbox-provider-fancybox.php (3)
future/includes/class-gv-utils.php (1)
Utils(12-193)includes/helper-functions.php (1)
gravityview_sanitize_html_class(140-156)future/includes/class-gv-entry.php (1)
get_slug(225-246)
⏰ 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: test_and_package
🔇 Additional comments (16)
includes/extensions/lightbox/class-gravityview-lightbox-provider.php (6)
95-107: Good deprecation path.Deprecation notice is explicit and points to the replacement hook.
109-119: Solid extension point.Empty base hook for inline assets keeps child providers clean and focused.
167-168: Hooks cleanup looks correct.Removes the new registration hooks as expected.
239-260: Guarded, idempotent enqueue is good.Slug checks and avoiding double-enqueue look correct. Inline injection after a fresh enqueue is the right spot.
267-278: Style enqueue guard looks good.Avoids duplicates and no-ops on empty handles.
363-366: On-demand enqueue aligns with objectives.Enqueuing when rendering links prevents unnecessary loads. Combined with early registration, this should resolve order conflicts with DataTables.
Please confirm FancyBox CSS reliably outputs when enqueued during content render across common themes. If any theme doesn’t print late styles, we may need a fallback (e.g., ensuring style is enqueued earlier when the provider is active).
includes/extensions/lightbox/fancybox/class-gravityview-lightbox-provider-fancybox.php (10)
52-61: Registered-before-inline guard is sensible.Bailing when handles aren’t registered prevents orphaned inline assets.
64-69: Safe JSON encoding path.wp_json_encode() check avoids malformed inline JS.
70-77: Inline init: ensure footer-friendly loading.Binding after the library load is correct. To guarantee the script prints when enqueued post-head, the handle should be registered for the footer (see suggestion below).
78-86: Inline CSS is fine; watch late-style printing.Inline styles attach to the handle correctly. If any theme fails to print late-enqueued styles, consider enqueuing the stylesheet earlier or adding a small runtime <style> as a fallback.
89-98: Back-compat method handled well.Clear deprecation plus delegation to the new flow.
148-150: Style registration looks good.Versioning and handle naming are consistent.
178-198: Defensive checks are solid; minor nit on early returns.Validations (lightbox enabled, details array, non-empty file_path) prevent noise. Converting non-array $link_atts is helpful.
200-202: Class merge and sanitation look good.gravityview_sanitize_html_class prevents unsafe class injections.
204-212: Gallery grouping logic: sanitize and uniqueness LGTM.Using form_id, field_id, and entry slug yields stable group IDs. esc_attr on parts is appropriate.
222-226: Verify FancyBox “pdf” data-type.Setting data-type to 'pdf' for SVGs and gv-iframe URLs may be FancyBox-version-specific. Many versions expect 'iframe' for PDFs and similar. Please confirm your bundled FancyBox supports 'pdf'; otherwise switch to 'iframe'.
Apply if needed:
- $link_atts['data-type'] = 'pdf'; + $link_atts['data-type'] = 'iframe';
| $this->enqueue_scripts(); | ||
| } |
There was a problem hiding this comment.
Styles aren’t enqueued when the provider is active (script only).
print_scripts_if_active() enqueues scripts, but not styles. On pages where no individual link triggers filter_entry_link_output(), styles may never be queued (e.g., AJAX-only content). Enqueue both here.
Apply:
- $this->enqueue_scripts();
+ $this->enqueue_scripts();
+ $this->enqueue_styles();📝 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.
| $this->enqueue_scripts(); | |
| } | |
| $this->enqueue_scripts(); | |
| $this->enqueue_styles(); | |
| } |
🤖 Prompt for AI Agents
In includes/extensions/lightbox/class-gravityview-lightbox-provider.php around
lines 91-92, print_scripts_if_active() currently only calls
$this->enqueue_scripts(), so styles are never enqueued when the provider is
active (script-only). Modify the method to also call $this->enqueue_styles() (or
the appropriate style enqueue helper) so both scripts and styles are
registered/queued when the provider is active; if enqueue_styles() doesn't exist
yet, add a matching method that registers/enqueues the required CSS and ensure
calls are idempotent and only run when the provider is active.
| public function register_scripts() { | ||
| wp_register_script( self::$script_slug, plugins_url( 'assets/lib/fancybox/dist/fancybox.umd.js', GRAVITYVIEW_FILE ), array(), GV_PLUGIN_VERSION ); | ||
| } |
There was a problem hiding this comment.
Register FancyBox script in the footer to support late enqueue.
Without $in_footer = true, enqueuing after wp_head risks the script never printing. This is the main cause of “works sometimes” symptoms.
Apply:
- wp_register_script( self::$script_slug, plugins_url( 'assets/lib/fancybox/dist/fancybox.umd.js', GRAVITYVIEW_FILE ), array(), GV_PLUGIN_VERSION );
+ wp_register_script( self::$script_slug, plugins_url( 'assets/lib/fancybox/dist/fancybox.umd.js', GRAVITYVIEW_FILE ), array(), GV_PLUGIN_VERSION, true );📝 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.
| public function register_scripts() { | |
| wp_register_script( self::$script_slug, plugins_url( 'assets/lib/fancybox/dist/fancybox.umd.js', GRAVITYVIEW_FILE ), array(), GV_PLUGIN_VERSION ); | |
| } | |
| public function register_scripts() { | |
| wp_register_script( self::$script_slug, plugins_url( 'assets/lib/fancybox/dist/fancybox.umd.js', GRAVITYVIEW_FILE ), array(), GV_PLUGIN_VERSION, true ); | |
| } |
🤖 Prompt for AI Agents
In
includes/extensions/lightbox/fancybox/class-gravityview-lightbox-provider-fancybox.php
around lines 141 to 143, wp_register_script is called without the $in_footer
flag which can prevent the script from printing when enqueued late; update the
wp_register_script call to pass the $in_footer parameter as true (i.e. set the
5th argument to true while keeping the existing dependencies and version) so the
FancyBox script is registered to load in the footer.
This implements #2450.
Summary
Refactors the lightbox provider's script enqueuing to resolve conflicts with DataTables rendering and follow WordPress best practices.
Problem
The lightbox provider was using outdated patterns that caused:
Solution
1. ✅ Proper WordPress Enqueuing Pattern
2. ✅ Modern WordPress APIs
<script>/<style>tags withwp_add_inline_script/style()3. ✅ Enhanced Security & Validation
esc_js()andesc_attr()for dynamic content4. ✅ Backward Compatibility
print_scripts()properly with_deprecated_function()add_inline_scripts_and_styles()for clarityTesting Checklist
Files Changed
includes/extensions/lightbox/class-gravityview-lightbox-provider.phpincludes/extensions/lightbox/fancybox/class-gravityview-lightbox-provider-fancybox.phpPerformance Impact
Security Improvements
Fixes #2450
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
💾 Build file (c6996c4).