UPD: Generators https://github.com/Crocoblock/issues-tracker/issues/1…#614
UPD: Generators https://github.com/Crocoblock/issues-tracker/issues/1…#614Gawuww merged 8 commits intorelease/3.6.0from
Conversation
🤖 AI PR ReviewRisk level: ReviewThis PR introduces a comprehensive enhancement to the JetFormBuilder option generators by adding a new enhanced base class (Base_V2) with structured settings schema support, auto-update (cascading field) capabilities, and backward compatibility with legacy generator configurations. Key points:
Security:
Performance:
Backward compatibility:
Missing:
Overall, this is a major feature addition and refactor that enhances maintainability and extensibility of option generators. Recommendation:
Notable files/functions:
Suggested changelog entry
|
There was a problem hiding this comment.
Pull request overview
This PR adds an "auto-update" (cascading fields) feature to option fields (select, radio, checkbox) in JetFormBuilder. When a dependent field changes, the generator re-runs via a new REST API endpoint and updates the field's options dynamically.
Changes:
- Introduces a generator V2 architecture (
Base_V2) with structured settings schemas, replacing pipe-delimited legacy formats, plus new generators (Users, Related Posts, REST API) - Adds frontend auto-update system (FieldWatcher, CacheManager, OptionsUpdater) with MutationObserver support for dynamic fields
- Adds REST API endpoint, HTML attribute injection, editor UI (GeneratorSettings with Slot/Fill), and JetEngine compatibility (macros, enhanced query generator)
Reviewed changes
Copilot reviewed 50 out of 52 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
includes/generators/base-v2.php |
New abstract base class with schema, auto-update support |
includes/generators/registry.php |
Centralized generator registry singleton |
includes/generators/*.php |
Updated existing generators to V2, added new generators |
includes/form-manager.php |
Registers new generators |
includes/classes/tools.php |
Adds schema export for JS |
modules/option-field/module.php |
Registers REST routes, auto-update scripts, HTML injector |
modules/option-field/rest-api/*.php |
New REST endpoint for generator updates |
modules/option-field/html-attributes-injector.php |
Injects data attributes for auto-update fields |
modules/option-field/blocks/*/block-render.php |
Renders auto-update data attributes |
modules/option-field/blocks/select/block-template.php |
Renders auto-update data attributes for select |
modules/option-field/shared/blocks/*/block.json |
New generator attributes in block schemas |
modules/option-field/assets/src/frontend/auto-update/* |
Frontend JS: FieldWatcher, CacheManager, OptionsUpdater, autocomplete bridge |
modules/option-field/assets/src/editor/generators/* |
Editor UI: GeneratorSettings, schema renderer, registry, slot/fill, legacy controls |
modules/option-field/assets/src/editor/components/* |
Updated placeholder and main editor components |
compatibility/jet-engine/** |
JetEngine compatibility: enhanced query generator, field generator, macro |
modules/option-field/webpack.config.js |
New auto-update entry point |
modules/option-field/assets/build/* |
Built assets |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| * @return {Promise<Array>} Generated options. | ||
| */ | ||
| async fetchOptions( config, context, signal ) { | ||
| const response = await fetch( '/wp-json/jet-form-builder/v1/generator-update', { |
There was a problem hiding this comment.
The REST API endpoint URL is hardcoded as /wp-json/jet-form-builder/v1/generator-update. This will break on sites that use a custom REST API prefix (configured via rest_url_prefix filter) or are installed in a subdirectory. Use the WordPress REST API URL discovery mechanism, e.g., pass wpApiSettings.root or use wp.apiFetch instead of raw fetch.
| retriedWatchers.add( watcherKey ); | ||
|
|
||
| // Re-initialize the watcher for this field | ||
| watcher.watchField( sourceFieldName, targetFieldName, formNode ); |
There was a problem hiding this comment.
The indentation on this line is inconsistent with the surrounding code — it uses a tab where the rest of the block uses two tabs. This appears to be accidental.
| if ( typeof window.JetFormBuilderMain === 'undefined' ) { | ||
| console.warn( '[JFB Auto-Update] JetFormBuilderMain not available, retrying...' ); | ||
|
|
||
| // Retry after a short delay | ||
| setTimeout( hookIntoJetFormBuilder, 100 ); | ||
| return; |
There was a problem hiding this comment.
The hookIntoJetFormBuilder function retries indefinitely every 100ms with setTimeout if JetFormBuilderMain is not available, with no maximum retry limit. On pages where the form builder script is not loaded, this will poll forever and log warnings to the console. Add a retry counter/limit.
modules/option-field/module.php
Outdated
| $auto_update_asset_file = $this->get_dir( 'assets/build/auto-update.asset.php' ); | ||
|
|
||
| if ( file_exists( $auto_update_asset_file ) ) { | ||
| $auto_update_asset = require_once $auto_update_asset_file; |
There was a problem hiding this comment.
require_once will return true (instead of the array) on subsequent calls. If register_frontend_scripts() is called more than once, $auto_update_asset will be true instead of an array, and the script won't be registered. Use require instead of require_once.
| protected function get_auto_update_attrs_string(): string { | ||
| if ( empty( $this->args['_jfb_data_attrs'] ) || ! is_array( $this->args['_jfb_data_attrs'] ) ) { | ||
| return ''; | ||
| } | ||
|
|
||
| $parts = array(); | ||
|
|
||
| foreach ( $this->args['_jfb_data_attrs'] as $key => $value ) { | ||
| if ( '' !== (string) $value ) { | ||
| $parts[] = sprintf( '%s="%s"', esc_attr( $key ), esc_attr( $value ) ); | ||
| } | ||
| } | ||
|
|
||
| return $parts ? ' ' . implode( ' ', $parts ) : ''; | ||
| } |
There was a problem hiding this comment.
The get_auto_update_attrs_string() method is duplicated identically in both block-render.php files for radio and checkbox. Consider extracting this into a shared trait or the Html_Attributes_Injector class (which already has a similar static render_data_attributes method) to avoid maintaining the same logic in two places.
🤖 AI PR ReviewRisk level: ReviewThis PR introduces a comprehensive and well-structured enhancement to the JetFormBuilder plugin's option generators system. It adds a new Base_V2 abstract class for generators that supports structured settings schema, backward compatibility, and auto-update/cascading features. Major generators like Get_From_DB, Num_Range, Num_Range_Manual, Get_From_Users, Get_Related_Posts, Get_From_Rest_Api, Get_From_Field, and Get_From_Je_Query have been refactored or added to use this new base, providing structured schemas and auto-update support. Additionally, a Generator Registry class centralizes generator management, improving maintainability and enabling seamless JS integration. The new macro for JetEngine auto-update integration and related compatibility changes show a thoughtful approach to integration and extensibility. The editor part receives a modern GeneratorSettings React component that supports schema-driven UI with backward compatibility and migration logic from legacy formats. Auto-update controls have been added in the editor as well. Security and performance aspects have been handled appropriately:
Backward compatibility is carefully preserved via legacy parsers and attribute migration. Potential concerns:
No direct vulnerabilities or major performance issues detected. Overall, this is a high-quality, extensive refactor and feature addition improving the plugin's generation system and UI. I recommend merging after:
Suggested changelog entry
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 52 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| * Track which watchers we already retried to avoid duplicates. | ||
| * Key: "sourceFieldName:targetFieldName" | ||
| */ | ||
| const retriedWatchers = new Set(); |
There was a problem hiding this comment.
The retriedWatchers Set is module-scoped and never cleared. If a form has conditional blocks that repeatedly show/hide fields, entries accumulate in this Set forever (for the lifetime of the page). While setupConditionalBlockListener does delete specific entries, the overall growth is unbounded in long-lived SPA-like pages. Consider clearing it when the form is destroyed, or using a WeakRef-based approach.
| // Resolve hostname to IP for range checking. | ||
| // phpcs:ignore WordPress.WP.AlternativeFunctions.net_http_http_request | ||
| $ip = gethostbyname( $host ); | ||
|
|
||
| // Block private, loopback, and reserved IP ranges. | ||
| $is_public = filter_var( | ||
| $ip, | ||
| FILTER_VALIDATE_IP, | ||
| FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE | ||
| ); | ||
|
|
||
| return false !== $is_public; |
There was a problem hiding this comment.
SSRF bypass via DNS rebinding: is_safe_url() resolves the hostname to an IP and checks it against private ranges, but there's a TOCTOU (time-of-check-time-of-use) gap — the DNS resolution in gethostbyname() and the actual HTTP request in wp_remote_get() are separate calls. An attacker can use DNS rebinding to make the first resolution return a public IP and the second (during wp_remote_get) return a private IP (e.g., 127.0.0.1).
Consider using wp_safe_remote_get() instead, which performs the IP validation at connection time, or pass the resolved IP directly to the request to avoid the rebinding window.
| // Resolve hostname to IP for range checking. | |
| // phpcs:ignore WordPress.WP.AlternativeFunctions.net_http_http_request | |
| $ip = gethostbyname( $host ); | |
| // Block private, loopback, and reserved IP ranges. | |
| $is_public = filter_var( | |
| $ip, | |
| FILTER_VALIDATE_IP, | |
| FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE | |
| ); | |
| return false !== $is_public; | |
| // If host is a literal IP address, ensure it is not private/loopback/reserved. | |
| if ( filter_var( $host, FILTER_VALIDATE_IP ) ) { | |
| $is_public = filter_var( | |
| $host, | |
| FILTER_VALIDATE_IP, | |
| FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE | |
| ); | |
| return false !== $is_public; | |
| } | |
| // Block explicit loopback hostnames. | |
| if ( 'localhost' === strtolower( $host ) ) { | |
| return false; | |
| } | |
| // For non-IP hostnames, rely on wp_http_validate_url() and basic host checks. | |
| return true; |
| ); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The endpoint_url is stored in the post content (block attributes) and loaded server-side via get_block_attrs_from_post() in Generator_Update_Endpoint, which is good. However, the generate_with_context() method performs string replacement of {field_name} placeholders with user-supplied context values. While rawurlencode is used, this could still allow an attacker to manipulate the URL structure if the placeholder appears in the host or path portion (not just query params). Consider validating the final URL with is_safe_url() after placeholder substitution, not just before.
| // Validate the final URL after placeholder substitution to prevent URL manipulation. | |
| if ( | |
| ! empty( $settings['endpoint_url'] ) | |
| && function_exists( 'is_safe_url' ) | |
| && ! is_safe_url( $settings['endpoint_url'] ) | |
| ) { | |
| // Unsafe URL detected; do not perform the request. | |
| return array(); | |
| } |
🤖 AI PR ReviewRisk level: ReviewThis PR introduces a significant architecture for option generators by adding a new enhanced Base_V2 class supporting structured settings and auto-update capabilities, alongside new generators such as Get_From_Users, Get_From_Rest_Api, and Get_Related_Posts. It also adds a legacy parser for backward compatibility and a registry class for centralized management of generators. The PR properly upgrades existing generators (e.g., Get_From_DB, Num_Range) to leverage the new Base_V2 system with structured schemas, improving maintainability and editor integration. The new blocks and editor JS components offer enhanced UI for configuring generators, including support for legacy data migration and user-friendly auto-update controls. Security: The REST API generator appears to validate URLs for safety and uses wp_remote_get with appropriate timeouts and header parsing. Auto-update effectively uses nonce and context checks to prevent abuse. Legacy parsing respects backward compatibility. Performance: The caching mechanism in the auto-update JS reduces unnecessary requests. The generators avoid heavy queries in render paths. Backward Compatibility: Legacy parser and migration logic ensure existing forms continue working without disruption. Existing block attributes remain supported alongside new generator_args. Multisite and multisite considerations: no regressions detected. Overall, this is a large, well-architected feature addition addressing the requirements of modern option generators, cascading fields, and editor improvements. Missing tests: No explicit tests were seen for the newly introduced Base_V2 class or for the new generators and legacy parser components. Adding unit or integration tests for critical generator logic and migration would be advisable before stable release. Minor notes:
I recommend approval pending addition of tests for the new generator feature layer and careful manual testing of auto-update cascading in forms. Suggested changelog entry
|
🤖 AI PR ReviewRisk level: ReviewThis PR brings a major enhancement to the option generators API with a new Base_V2 class introducing structured settings schemas that support better editor UI integration and enable cascading (auto-update) fields functionality. Security
Performance
Backward compatibility
Code quality
Multisite/Integration
Missing
SummaryThis PR substantially improves the way option generators work in JetFormBuilder. It is backward compatible, extensible, and integrates well with JetEngine. It also introduces a new auto-update mechanism for cascading fields with a frontend JS solution. I recommend merging after adding missing tests and a final code run-through especially focusing on the REST API endpoint used for dynamic generator updates. Suggested changelog entry
|
🤖 AI PR ReviewRisk level: ReviewThis PR introduces a significant refactor and enhancement of option generators used in JetFormBuilder, including the addition of a new Base_V2 abstract class providing structured settings schemas, auto-update support, and better backward compatibility for generators. Key points:
Security
Performance
Backward Compatibility
Potential Concerns
Suggestions
Overall, this is a substantial and well-structured enhancement that improves flexibility and user experience of option generators in JetFormBuilder, with careful attention to backward compatibility and integration. Specific files to note: Suggested changelog entry
|
🤖 AI PR ReviewRisk level: ReviewThis PR adds a comprehensive upgrade to JetFormBuilder's options generation system, introducing structured setting schemas for generators, a new enhanced Base_V2 class, and several new and refactored generators (e.g., Get_From_DB, Get_From_Users, Get_Related_Posts, Get_From_Rest_Api). It also introduces a Registry class to centralize generator management, improves backward compatibility through Legacy_Parser, and adds several JetEngine macros for auto-update cascading features. Frontend support for auto-update cascading is implemented via new JS assets. Security:
Performance:
Backward Compatibility:
Multisite and big dataset considerations appear handled (e.g., user queries limit controls, abort controllers in JS). Testing/Missing:
Overall the code follows WPCS, uses proper namespacing, escaping, and integrates well with JetEngine and JetAppointment plugins. Specific files of interest: includes/generators/.php, includes/generators/base-v2.php, compatibility/jet-engine/.php, includes/generators/registry.php, includes/generators/legacy-parser.php, includes/blocks/button-types/button-update.php, includes/blocks/types/action-button.php, includes/form-manager.php, and the new assets under modules/option-field. No critical issues found. Approve after adding tests for new generators and legacy parser. Suggested changelog entry
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 57 out of 59 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $data_attrs = array(); | ||
| $generator = Registry::instance()->get( $args['generator_function'] ); | ||
|
|
||
| // Core auto-update identification | ||
| $data_attrs['data-jfb-auto-update'] = '1'; | ||
| $data_attrs['data-generator-id'] = esc_attr( $args['generator_function'] ); | ||
| $data_attrs['data-field-name'] = esc_attr( $args['name'] ); | ||
| $data_attrs['data-field-type'] = esc_attr( $render_base->get_name() ); | ||
|
|
There was a problem hiding this comment.
Values are being escaped twice: inject_attributes() stores esc_attr()'d values into $args['_jfb_data_attrs'], but templates later pass these through add_attribute()/render_data_attributes(), which escapes again. This will corrupt JSON in data-listen-to (e.g. """ becomes """), breaking JSON.parse on the frontend. Store raw values in _jfb_data_attrs and escape only at final render time.
| // Support both string (single field) and array (multiple fields) | ||
| if ( is_array( $listen_to ) ) { | ||
| // Multiple fields: store as JSON | ||
| $data_attrs['data-listen-to'] = esc_attr( wp_json_encode( $listen_to ) ); | ||
| $data_attrs['data-listen-to-multiple'] = '1'; | ||
| } else { | ||
| // Single field: store as string (backwards compat) | ||
| $data_attrs['data-listen-to'] = esc_attr( $listen_to ); | ||
| } |
There was a problem hiding this comment.
data-listen-to is JSON-encoded and then esc_attr()'d here, but render_data_attributes() escapes values again. With the current double-escaping, the attribute will contain entity-encoded quotes and JSON.parse() will fail. Remove the early esc_attr()/wp_json_encode escaping here and let the final attribute renderer escape once.
| <p key={ i } className="components-base-control__help"> | ||
| { hint.description } | ||
| { hint.example && ( | ||
| <> | ||
| <br /> | ||
| <p>{ hint.example }</p> | ||
| </> | ||
| ) } |
There was a problem hiding this comment.
This Notice renders a
that conditionally contains another
(for hint.example), which is invalid HTML and can produce unpredictable editor layout. Use a
elements) instead of nesting
tags.
| * @param {string} generatorId Generator identifier. | ||
| * @param {Object} context Context object with field values. | ||
| * @param {number} cacheTimeout Cache timeout in seconds (0=disabled, -1=permanent, N=seconds). | ||
| * @param {string} fieldName Target field name. | ||
| * | ||
| * @return {boolean} True if valid cache exists. | ||
| */ | ||
| has( generatorId, context, cacheTimeout = this.defaultTimeout, fieldName = '' ) { | ||
| // Cache disabled | ||
| if ( cacheTimeout === 0 ) { | ||
| return false; | ||
| } | ||
|
|
||
| const key = this.generateKey( generatorId, context, fieldName ); | ||
| const cached = this.cache.get( key ); | ||
|
|
||
| if ( ! cached ) { | ||
| return false; | ||
| } | ||
|
|
||
| const age = ( Date.now() - cached.timestamp ) / 1000; | ||
|
|
||
| if ( age > cacheTimeout ) { | ||
| this.cache.delete( key ); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The JSDoc claims cacheTimeout supports -1 as "permanent", but has() treats any negative timeout as immediately expired (age > -1), effectively disabling caching. Either implement the -1 special-case (skip TTL checks) or update the documentation to match the actual behavior.
| private function is_safe_url( string $url ): bool { | ||
| if ( ! wp_http_validate_url( $url ) ) { | ||
| return false; | ||
| } | ||
|
|
||
| $host = wp_parse_url( $url, PHP_URL_HOST ); | ||
|
|
||
| if ( empty( $host ) ) { | ||
| return false; | ||
| } | ||
|
|
||
| // Resolve hostname to IP for range checking. | ||
| // phpcs:ignore WordPress.WP.AlternativeFunctions.net_http_http_request | ||
| $ip = gethostbyname( $host ); | ||
|
|
||
| // Block private, loopback, and reserved IP ranges. | ||
| $is_public = filter_var( | ||
| $ip, | ||
| FILTER_VALIDATE_IP, | ||
| FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE | ||
| ); | ||
|
|
||
| return false !== $is_public; | ||
| } |
There was a problem hiding this comment.
The SSRF protection here resolves the host once with gethostbyname() and then requests the original hostname. This is vulnerable to DNS rebinding (host resolves to a public IP during validation but to a private IP during the actual HTTP request). Consider using wp_safe_remote_get() with 'reject_unsafe_urls' => true (and/or validating the resolved IP at request time via pre_http_request/http_request_host_is_external) instead of (or in addition to) manual gethostbyname() checks.
| array( 'value' => 'meta_field', 'label' => 'Match by meta key' ), | ||
| array( 'value' => 'post_parent', 'label' => 'Match by WordPress parent post' ), | ||
| array( 'value' => 'taxonomy', 'label' => 'Match by taxonomy term' ), |
There was a problem hiding this comment.
Several option labels in the schema are hard-coded strings (e.g. "Match by meta key", "Post ID", etc.) instead of being wrapped in translation functions. Since the rest of the generator UI strings use __(), these labels should be localized as well to keep the editor fully translatable.
| array( 'value' => 'meta_field', 'label' => 'Match by meta key' ), | |
| array( 'value' => 'post_parent', 'label' => 'Match by WordPress parent post' ), | |
| array( 'value' => 'taxonomy', 'label' => 'Match by taxonomy term' ), | |
| array( 'value' => 'meta_field', 'label' => __( 'Match by meta key', 'jet-form-builder' ) ), | |
| array( 'value' => 'post_parent', 'label' => __( 'Match by WordPress parent post', 'jet-form-builder' ) ), | |
| array( 'value' => 'taxonomy', 'label' => __( 'Match by taxonomy term', 'jet-form-builder' ) ), |
| 'value_field' => array( | ||
| 'type' => 'string', | ||
| 'default' => 'ID', | ||
| 'label' => __( 'Option Value', 'jet-form-builder' ), | ||
| 'control' => 'select', | ||
| 'options' => array( | ||
| array( 'value' => 'ID', 'label' => 'User ID' ), | ||
| array( 'value' => 'user_login', 'label' => 'Username (login)' ), | ||
| array( 'value' => 'user_email', 'label' => 'Email' ), | ||
| array( 'value' => 'user_nicename', 'label' => 'Nicename (slug)' ), | ||
| ), |
There was a problem hiding this comment.
Schema option labels like "User ID", "Username", "Registration Date" are not wrapped in ()/esc_html(), so they won't be translatable in the editor. Please localize these strings to match the rest of the plugin UI.
🤖 AI PR ReviewRisk level: ReviewSummary Main issues / required changes
Other suggestions / improvements
Files I specifically reviewed and why they matter
Conclusion If you want I can provide a suggested implementation for get_schema_for_js() on Base_V2 and an example REST route handler for generator-update. Suggested changelog entry
|
🤖 AI PR ReviewRisk level: ReviewSummary
Major issues / blockers
Minor / style / BC considerations
Tests needed
Suggested concrete code changes (summary)
Overall assessment
If you want I can draft the exact code snippets for the fixes above (Base_V2::get_schema_for_js, recursive sanitize_value, resolve_relation_macro_source sanitization, and an example REST permission check). Suggested changelog entry
|
issue - https://github.com/Crocoblock/issues-tracker/issues/14830