Add Headline Testing Setup to Parse.ly plugin #3601
Conversation
…stead of direct output
…on, fix array type annotations, and update REST API return type
…er type casting, and fix return type annotations
…rs to avoid wp_script_add_data array issue
…ons and remove allowed_user_roles validation
…eters when live updates are enabled
📝 WalkthroughWalkthroughAdds a Headline Testing feature with UI fields, REST settings endpoint, option defaults, and frontend script loading (one_line and advanced). Wires the feature into bootstrap. Adds nested option support in settings UI, an early WordPress guard in telemetry init, and makes Lint_Rule’s fix_pattern constructor parameter required. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant WP as WordPress
participant Bootstrap as wp-parsely.php
participant Parsely as Parsely Core
participant HT as Headline_Testing
participant UI as Settings UI / REST
WP->>Bootstrap: Plugin loaded
Bootstrap->>HT: new Headline_Testing(Parsely)
Bootstrap->>HT: run()
HT->>Parsely: read options (headline_testing) and site_id
HT->>HT: should_initialize() / is_enabled()
alt enabled & permitted
HT->>WP: add_action(wp_enqueue_scripts, enqueue_headline_testing_script)
HT->>WP: add_action(admin_enqueue_scripts, enqueue_admin_scripts)
else disabled
HT-->>Bootstrap: no hooks registered
end
WP->>HT: wp_enqueue_scripts
alt installation_method == one_line
HT->>WP: register external script (VIPExp URL)
HT->>WP: add_filter(script_loader_tag) to add data attrs
HT->>WP: wp_enqueue_script
else installation_method == advanced
HT->>WP: register empty handle
HT->>WP: wp_add_inline_script (VIPExp loader + config)
HT->>WP: wp_enqueue_script
end
UI-->>Parsely: REST/UI read/write headline_testing settings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.17)Note: Using configuration file /phpstan.neon. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (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
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (2)
lint.php (2)
124-124: Update conditional check after making fix_pattern mandatory.The null check
null !== $this->fix_patternis now redundant since fix_pattern is required and can never be null. This condition will always evaluate to true.Apply this diff to remove the redundant null check:
-if ( $auto_fix_mode && null !== $this->fix_pattern ) { +if ( $auto_fix_mode ) {
149-149: Update display logic after making fix_pattern mandatory.The condition
null !== $this->fix_patternis redundant since fix_pattern can never be null now.Apply this diff to remove the redundant check:
-if ( null !== $this->fix_pattern ) { +// All rules now support auto-fixing
🧹 Nitpick comments (8)
lint.php (2)
84-84: Update docblock to reflect mandatory parameter.The docblock still indicates that
$fix_patternis optional, but the parameter is now required.Apply this diff to update the docblock:
- * @param string|null $fix_pattern The regex to fix any violations (optional). + * @param string $fix_pattern The regex to fix any violations.
62-64: Update property docblock to reflect non-nullable type.The property docblock still indicates that
$fix_patterncan be null, but it's now always a string.Apply this diff to update the property docblock:
- * @var string|null Optional regex pattern used to auto-fix the violation. + * @var string Regex pattern used to auto-fix the violation.src/class-headline-testing.php (2)
94-116: Optimize script enqueuing with early returns.The method has good validation logic, but the enabled check is duplicated from should_initialize(). Consider removing this duplication since run() already gates on should_initialize().
Apply this diff to remove the duplicate check:
public function enqueue_headline_testing_script(): void { $options = $this->parsely->get_options(); - - // Check if headline testing is enabled. - if ( false === $options['headline_testing']['enabled'] ) { - return; - } - + $headline_testing_options = $options['headline_testing']; $site_id = $this->parsely->get_site_id();
221-223: Consider implementing admin-specific functionality.The enqueue_admin_scripts() method is currently empty. If admin-specific scripts are needed for headline testing management or preview, implement them here.
Do you need help implementing admin-specific functionality for headline testing, such as preview capabilities or administrative controls?
src/rest-api/settings/class-endpoint-headline-testing-settings.php (2)
13-13: Remove unnecessary use statementThe
Base_Settings_Endpointclass is already in the same namespace, so the use statement is redundant.-use Parsely\REST_API\Settings\Base_Settings_Endpoint; -
31-31: Add missing blank line after methodAccording to WordPress coding standards, there should be a blank line between methods.
} + /**src/UI/class-settings-page.php (2)
975-997: Complex nested option handling - consider refactoringThe nested option handling logic is becoming complex with multiple conditional branches for different option groups. Consider extracting this into a separate helper method for better maintainability.
public function print_text_tag( $args ): void { $options = $this->parsely->get_options(); $name = $args['option_key']; - // Get option value - handle nested options properly. - if ( false === strpos( $name, '[' ) ) { - $raw_value = $options[ $name ] ?? ''; - $value = is_scalar( $raw_value ) ? (string) $raw_value : ''; - } else { - $raw_value = Parsely::get_nested_option_value( $name, $options ) ?? ''; - $value = is_scalar( $raw_value ) ? (string) $raw_value : ''; - } + $value = $this->get_option_value_for_display( $name, $options ); $optional_args = $args['optional_args'] ?? array(); $id = esc_attr( $name ); - // Handle nested option names properly. - if ( strpos( $name, 'headline_testing' ) === 0 ) { - $html_name = str_replace( - 'headline_testing', - '[headline_testing]', - Parsely::OPTIONS_KEY . esc_attr( $name ) - ); - } else { - $html_name = Parsely::OPTIONS_KEY . '[' . esc_attr( $name ) . ']'; - } + $html_name = $this->get_html_name_attribute( $name );Add these helper methods:
private function get_option_value_for_display( string $name, array $options ): string { if ( false === strpos( $name, '[' ) ) { $raw_value = $options[ $name ] ?? ''; } else { $raw_value = Parsely::get_nested_option_value( $name, $options ) ?? ''; } return is_scalar( $raw_value ) ? (string) $raw_value : ''; } private function get_html_name_attribute( string $name ): string { if ( strpos( $name, 'headline_testing' ) === 0 ) { return str_replace( 'headline_testing', '[headline_testing]', Parsely::OPTIONS_KEY . esc_attr( $name ) ); } return Parsely::OPTIONS_KEY . '[' . esc_attr( $name ) . ']'; }
1620-1624: Remove redundant commentsThe comments about redundant checks acknowledge they're unnecessary since the merge with defaults already handles this. Consider removing these redundant lines and comments.
- // Add any missing data due to unchecked checkboxes. - // These checks are redundant since we already merged with defaults above, - // but we keep them for safety in case the merge didn't work as expected. - // No need for null coalescing since the keys are guaranteed to exist after the merge. -
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
lint.php(1 hunks)src/Telemetry/telemetry-init.php(1 hunks)src/UI/class-settings-page.php(12 hunks)src/class-headline-testing.php(1 hunks)src/class-parsely.php(3 hunks)src/rest-api/settings/class-endpoint-headline-testing-settings.php(1 hunks)src/rest-api/settings/class-settings-controller.php(2 hunks)wp-parsely.php(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{html,php}
⚙️ CodeRabbit Configuration File
**/*.{html,php}: "Perform a detailed review of the provided code with following key aspects in mind:
- Review the HTML and PHP code to ensure it is well-structured and adheres to best practices.
- Ensure the code follows WordPress coding standards and is well-documented.
- Confirm the code is secure and free from vulnerabilities.
- Optimize the code for performance, removing any unnecessary elements.
- Validate comments for accuracy, currency, and adherence to WordPress coding standards.
- Ensure each line comment concludes with a period.
- Verify code compatibility with the latest version of WordPress, avoiding deprecated functions or features."
Files:
src/Telemetry/telemetry-init.phpwp-parsely.phpsrc/rest-api/settings/class-settings-controller.phpsrc/rest-api/settings/class-endpoint-headline-testing-settings.phpsrc/class-parsely.phpsrc/class-headline-testing.phplint.phpsrc/UI/class-settings-page.php
🧠 Learnings (1)
📚 Learning: 2024-10-12T10:01:08.699Z
Learnt from: acicovic
PR: Parsely/wp-parsely#2554
File: src/class-parsely.php:463-483
Timestamp: 2024-10-12T10:01:08.699Z
Learning: The `get_nested_option_value` method in `src/class-parsely.php` is intentionally designed without error handling for non-existent keys to meet specific design criteria.
Applied to files:
src/UI/class-settings-page.php
🧬 Code Graph Analysis (4)
src/rest-api/settings/class-settings-controller.php (2)
src/class-parsely.php (1)
Parsely(85-1210)src/rest-api/settings/class-endpoint-headline-testing-settings.php (1)
Endpoint_Headline_Testing_Settings(20-77)
src/rest-api/settings/class-endpoint-headline-testing-settings.php (2)
src/class-parsely.php (1)
Parsely(85-1210)src/rest-api/settings/class-base-settings-endpoint.php (1)
Base_Settings_Endpoint(28-414)
src/class-headline-testing.php (3)
src/class-parsely.php (4)
Parsely(85-1210)run(290-312)get_options(568-628)get_site_id(840-844)src/content-helper/common/class-content-helper-feature.php (1)
Content_Helper_Feature(22-227)src/class-permissions.php (1)
Permissions(21-215)
src/UI/class-settings-page.php (1)
src/class-parsely.php (3)
Parsely(85-1210)get_nested_option_value(639-650)get_options(568-628)
🔇 Additional comments (22)
src/Telemetry/telemetry-init.php (1)
14-20: No file-scope WordPress calls found; guard placement is safeI inspected src/Telemetry/class-telemetry-system.php for top-level WP functions and found none, so loading it before the
add_actioncheck poses no risk today. The suggested reorder is purely defensive—no change is required.src/class-headline-testing.php (7)
1-11: LGTM - Well-structured file header and namespace declaration.The file follows WordPress coding standards with proper docblock, strict types declaration, and appropriate namespace usage.
23-38: LGTM - Class structure follows Content Helper feature pattern.The class properly extends Content_Helper_Feature and follows established patterns. The Parsely dependency injection in the constructor is appropriate.
45-53: LGTM - Feature initialization follows established patterns.The run() method correctly uses the can_enable_feature() helper and registers appropriate hooks for both frontend and admin functionality.
126-171: LGTM - One-line script implementation is well-structured.The method properly constructs the script URL, handles data attributes conditionally, and uses WordPress script registration/enqueuing correctly. The script_loader_tag filter approach for adding data attributes is appropriate.
181-214: LGTM - Advanced script implementation handles configuration properly.The method correctly builds configuration options and injects the inline VIP experiments script. The script content appears to be a minified version of the headline testing loader, which is appropriate.
246-270: LGTM - Static methods follow Content Helper feature interface.The required static methods properly return unique identifiers for the headline testing feature, maintaining consistency with the Content Helper architecture.
201-201: Validate and document the Parse.ly VIP_EXP loaderThe
VIP_EXPobject & inline script aren’t part of Parse.ly’s public API, so we need to confirm its origin, document its purpose, and improve how it’s loaded.• Confirm source & integrity
– Reach out to Parse.ly Support (support@parsely.com or your WP VIP RM) for the official headline-testing snippet or non-minified loader.
– Verify that the inlined code exactly matches the vendor-provided version.• External hosting & SRI
– Instead of inlining, host the script on a trusted CDN (or your own asset bucket) and include it via<script src="…" integrity="…" crossorigin="anonymous"></script>.
– This enables browser caching, integrity checks, and easier updates.• Documentation & debugging
– Add a comment block aboveloadVIPExpinsrc/class-headline-testing.phpdescribing:
- WhatVIP_EXP.configoptions (apikey, enableFlickerControl) do
- How flicker control works (hiding body until FCP)
- Where to find the unminified source for local debugging
– Keep a non-minified copy in your repo (or link to it) for development builds.src/rest-api/settings/class-settings-controller.php (2)
14-14: LGTM - Proper import statement for new endpoint class.The import follows the established pattern and namespace conventions for REST API endpoint classes.
45-45: LGTM - Endpoint registration follows established pattern.The new Endpoint_Headline_Testing_Settings is properly instantiated and added to the endpoints array, maintaining consistency with existing endpoint registrations.
wp-parsely.php (2)
34-34: LGTM - Proper import for Headline Testing class.The import statement follows the established pattern and is correctly placed in the alphabetical order of imports.
102-104: LGTM - Feature initialization follows established bootstrap pattern.The Headline Testing feature is properly instantiated and initialized after the Metadata_Renderer, following the same pattern as other plugin features. The placement ensures proper dependency order.
src/class-parsely.php (3)
35-35: LGTM - PHPStan type definition added to main options type.The headline_testing option is properly integrated into the main Parsely_Options type, maintaining type safety and consistency.
65-72: LGTM - Well-defined PHPStan type for headline testing options.The Parsely_Options_Headline_Testing type properly defines all required fields with appropriate types. The structure aligns with the REST API endpoint specifications and UI requirements.
149-156: LGTM - Appropriate default values for headline testing options.The default values are sensible:
- Feature disabled by default (enabled: false)
- Simple installation method as default (one_line)
- Conservative settings for performance-impacting features (flicker control, live updates disabled)
- Reasonable timeout value (30000ms = 30s)
- Safe default for content loading behavior
src/rest-api/settings/class-endpoint-headline-testing-settings.php (1)
49-76: Validate live_update_timeout range valuesThe
range()function generates an array of 60 values (from 1000 to 60000 in steps of 1000). Consider if this is the intended behavior - users might expect to enter any value within this range, not just multiples of 1000.If you want to allow any value between 1000 and 60000, consider using a validation callback in the base class instead of pre-generating all valid values.
src/UI/class-settings-page.php (6)
65-65: Type definition looks good for headline_testingThe PHPDoc type definition correctly specifies the structure of the headline_testing array with all its properties and their types.
533-537: LGTM: Method documentation follows WordPress standardsThe PHPDoc comment properly documents the new method with the correct @SInCE tag.
544-547: Security: Properly escaped outputThe section description text is correctly escaped using
esc_html__()for the translatable strings and the link is properly formatted with security attributes.
1287-1290: Helper method properly implements nested option retrievalGood implementation of the helper method for retrieving nested option values.
1583-1632: Validation logic looks comprehensiveThe headline testing validation properly:
- Initializes the array if missing
- Merges with defaults to ensure all keys exist
- Recursively sanitizes all values
- Uses a dedicated sanitization method for field-specific rules
1978-1994: Well-structured field sanitizationThe sanitization method properly handles each field type:
- Boolean fields are correctly coerced
- Installation method validates against allowed values
- Timeout value is validated within the expected range with proper fallback
- Default case handles unexpected fields safely
| string $error_pattern, | ||
| string $include_files, | ||
| string $fix_pattern = null, | ||
| string $fix_pattern, |
There was a problem hiding this comment.
Constructor parameter signature change makes fix_pattern mandatory.
The change from string|null $fix_pattern = null to string $fix_pattern makes the fix pattern required at construction time. This is a breaking change that affects all existing Lint_Rule instantiations.
Verify that all existing usages of the Lint_Rule constructor have been updated to provide the fix_pattern parameter:
#!/bin/bash
# Description: Search for all Lint_Rule constructor calls and verify they provide fix_pattern
# Expected: All constructor calls should have 4 parameters (after the change)
echo "=== Searching for Lint_Rule constructor calls ==="
rg -A 3 -B 1 "new Lint_Rule\(" --type php
echo -e "\n=== Checking instantiation patterns ==="
ast-grep --pattern 'new Lint_Rule($$$)'🤖 Prompt for AI Agents
In lint.php around line 91 the constructor signature was changed to require
string $fix_pattern which breaks existing Lint_Rule instantiations; either
restore the original nullable default by changing the parameter back to
"string|null $fix_pattern = null" to preserve backward compatibility, or (if you
intend the breaking change) update every "new Lint_Rule(...)" call site to pass
the fix_pattern argument; ensure you choose one approach, update the constructor
signature or all call sites consistently, and run a repo-wide search for "new
Lint_Rule(" to verify no instantiations are left without the fix_pattern
parameter.
| private function user_has_permission(): bool { | ||
| // For headline testing, we'll allow all users since it's a frontend feature. | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing proper permission checking.
The user_has_permission() method currently returns true for all users. For a feature that loads external scripts, consider implementing proper permission checks to ensure only authorized users can trigger the headline testing functionality.
Apply this diff to implement basic permission checking:
private function user_has_permission(): bool {
- // For headline testing, we'll allow all users since it's a frontend feature.
- return true;
+ // Allow users who can edit posts or have been explicitly granted permission
+ return current_user_can( 'edit_posts' ) || apply_filters( 'wp_parsely_headline_testing_user_permission', false );
}📝 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.
| private function user_has_permission(): bool { | |
| // For headline testing, we'll allow all users since it's a frontend feature. | |
| return true; | |
| } | |
| private function user_has_permission(): bool { | |
| // Allow users who can edit posts or have been explicitly granted permission | |
| return current_user_can( 'edit_posts' ) || apply_filters( 'wp_parsely_headline_testing_user_permission', false ); | |
| } |
🤖 Prompt for AI Agents
In src/class-headline-testing.php around lines 84 to 87, user_has_permission()
currently returns true for everyone; replace it with a proper permission check
that ensures only logged-in users with an appropriate capability can trigger the
feature. Implement: return is_user_logged_in() && current_user_can('edit_posts')
(or a stricter capability like 'manage_options' if needed) and wrap the result
in a filter (e.g. apply_filters('headline_testing_user_can', $allowed, $user))
so other code can override the policy.
| $field_id = 'headline_testing[live_update_timeout]'; | ||
| $field_args = array( | ||
| 'option_key' => $field_id, | ||
| 'label_for' => $field_id, | ||
| 'help_text' => __( 'How long to watch for new content (in milliseconds). Default: 30000 (30 seconds).', 'wp-parsely' ), | ||
| 'optional_args' => array( | ||
| 'type' => 'number', | ||
| 'placeholder' => '30000', | ||
| 'min' => '1000', | ||
| 'max' => '60000', | ||
| 'step' => '1000', | ||
| ), | ||
| ); | ||
| add_settings_field( | ||
| $field_id, | ||
| __( 'Live Update Timeout (ms)', 'wp-parsely' ), | ||
| array( $this, 'print_text_tag' ), | ||
| Parsely::MENU_SLUG, | ||
| $section_key, | ||
| $field_args | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify HTML5 input attributes for number field
The number input field includes min, max, and step attributes which are good for validation. However, ensure these are properly rendered in the print_text_tag method.
🏁 Script executed:
#!/bin/bash
# Check if print_text_tag properly handles min, max, and step attributes
ast-grep --pattern 'function print_text_tag($_) {
$$$
}'Length of output: 60
🏁 Script executed:
#!/bin/bash
# Locate the print_text_tag method and show its implementation for review
rg -n "function print_text_tag" -n src/UI/class-settings-page.php
rg -A20 -n "function print_text_tag" src/UI/class-settings-page.phpLength of output: 1041
🏁 Script executed:
#!/bin/bash
# Show the full implementation of print_text_tag to verify handling of optional_args
rg -A50 -n "public function print_text_tag" src/UI/class-settings-page.phpLength of output: 2291
Add support for min, max, and step in print_text_tag
The print_text_tag method in src/UI/class-settings-page.php only emits placeholder, required and disabled. Your number field’s optional_args include min, max, and step, but those aren’t in the whitelist and therefore never rendered.
– File: src/UI/class-settings-page.php
– Around line 1000
Suggested fix: extend the $accepted_args array to include these HTML5 attributes (or drop the whitelist if you intend to allow arbitrary attributes). For example:
--- a/src/UI/class-settings-page.php
+++ b/src/UI/class-settings-page.php
@@ public function print_text_tag( $args ): void {
- $accepted_args = array( 'placeholder', 'required', 'disabled' );
+ $accepted_args = array( 'placeholder', 'required', 'disabled', 'min', 'max', 'step' );This will ensure your number input renders the min, max, and step attributes as expected.
📝 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.
| $field_id = 'headline_testing[live_update_timeout]'; | |
| $field_args = array( | |
| 'option_key' => $field_id, | |
| 'label_for' => $field_id, | |
| 'help_text' => __( 'How long to watch for new content (in milliseconds). Default: 30000 (30 seconds).', 'wp-parsely' ), | |
| 'optional_args' => array( | |
| 'type' => 'number', | |
| 'placeholder' => '30000', | |
| 'min' => '1000', | |
| 'max' => '60000', | |
| 'step' => '1000', | |
| ), | |
| ); | |
| add_settings_field( | |
| $field_id, | |
| __( 'Live Update Timeout (ms)', 'wp-parsely' ), | |
| array( $this, 'print_text_tag' ), | |
| Parsely::MENU_SLUG, | |
| $section_key, | |
| $field_args | |
| ); | |
| // In src/UI/class-settings-page.php, around the start of print_text_tag(): | |
| public function print_text_tag( $args ): void { | |
| $accepted_args = array( 'placeholder', 'required', 'disabled', 'min', 'max', 'step' ); | |
| // ...rest of the method remains unchanged. | |
| } |
🤖 Prompt for AI Agents
src/UI/class-settings-page.php around lines 625-645 (field registration) and
around line 1000 (print_text_tag): the number input's optional_args include min,
max and step but print_text_tag currently only emits placeholder, required and
disabled; extend the print_text_tag's $accepted_args whitelist to include 'min',
'max' and 'step' so those attributes are output for number inputs (ensure you
use the same escaping/sanitization used for other attributes when rendering
their values).
| $accepted_args = array( 'placeholder', 'required', 'disabled' ); | ||
| $type = $optional_args['type'] ?? 'text'; |
There was a problem hiding this comment.
Add support for min, max, and step attributes
The accepted_args array should include 'min', 'max', and 'step' to support the HTML5 number input attributes defined in the headline testing timeout field.
-$accepted_args = array( 'placeholder', 'required', 'disabled' );
+$accepted_args = array( 'placeholder', 'required', 'disabled', 'min', 'max', 'step' );📝 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.
| $accepted_args = array( 'placeholder', 'required', 'disabled' ); | |
| $type = $optional_args['type'] ?? 'text'; | |
| $accepted_args = array( 'placeholder', 'required', 'disabled', 'min', 'max', 'step' ); | |
| $type = $optional_args['type'] ?? 'text'; |
🤖 Prompt for AI Agents
In src/UI/class-settings-page.php around lines 1000-1001, the accepted_args
array currently only contains 'placeholder', 'required', and 'disabled' but
needs to accept the HTML5 number input attributes 'min', 'max', and 'step';
update the $accepted_args array to include 'min', 'max', and 'step' so those
attributes are passed through when rendering the headline testing timeout (and
any other numeric fields).
| if ( strpos( $name, 'content_helper' ) === 0 ) { | ||
| $html_name = str_replace( | ||
| 'content_helper', | ||
| '[content_helper]', | ||
| Parsely::OPTIONS_KEY . esc_attr( $name ) | ||
| ); | ||
| } elseif ( strpos( $name, 'headline_testing' ) === 0 ) { | ||
| $html_name = str_replace( | ||
| 'headline_testing', | ||
| '[headline_testing]', | ||
| Parsely::OPTIONS_KEY . esc_attr( $name ) | ||
| ); | ||
| } else { | ||
| $html_name = Parsely::OPTIONS_KEY . '[' . esc_attr( $name ) . ']'; | ||
| } | ||
| $yes_text = $args['yes_text'] ?? ''; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicated logic for HTML name generation
This logic for generating HTML names for nested options is duplicated from the print_text_tag method. Consider extracting to a shared helper method.
Use the same get_html_name_attribute() helper method suggested earlier to avoid code duplication.
🤖 Prompt for AI Agents
In src/UI/class-settings-page.php around lines 1044 to 1059, the HTML
name-generation branch (handling content_helper, headline_testing, and default
cases) duplicates logic from print_text_tag; replace this block with a call to
the shared get_html_name_attribute() helper (the same helper used by
print_text_tag). Remove the duplicated str_replace/Parsely::OPTIONS_KEY logic,
pass the original $name (and any required context) into
get_html_name_attribute(), and assign its return to $html_name; ensure the
helper handles escaping and the special prefixes so behavior remains identical.
| $name = $args['option_key']; | ||
| $id = esc_attr( $name ); | ||
| $selected = $this->get_nested_option_value( $name ); | ||
|
|
||
| // Handle nested option names properly. | ||
| if ( strpos( $name, 'headline_testing' ) === 0 ) { | ||
| $html_name = str_replace( | ||
| 'headline_testing', | ||
| '[headline_testing]', | ||
| Parsely::OPTIONS_KEY . esc_attr( $name ) | ||
| ); | ||
| } else { | ||
| $html_name = Parsely::OPTIONS_KEY . '[' . esc_attr( $name ) . ']'; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
More duplication of HTML name generation logic
This is the third occurrence of the same nested option name handling logic. This confirms the need for a shared helper method.
Reuse the get_html_name_attribute() helper method here as well.
🤖 Prompt for AI Agents
In src/UI/class-settings-page.php around lines 1201 to 1214, the HTML name
attribute is being computed with duplicated nested-option logic; replace the
entire conditional block with a single call to the shared helper by setting
$html_name = $this->get_html_name_attribute( $name ); (or pass
$args['option_key'] directly) and remove the manual str_replace/concat code so
the helper centralizes escaping and nested-name formatting.
Co-authored-by: Sarosh Aga <sarosh.aga@automattic.com>
Description
This change introduces a new tab to the Parse.ly Settings page in the WP Admin area focused around Headline Testing.
It covers all available integration options and configurations we do currently list on https://docs.parse.ly/install-headline-testing/
Motivation and context
How has this been tested?
How to test
wp-adminand the Parse.ly Menu ItemScreenshots (if appropriate)
Summary by CodeRabbit
New Features
Bug Fixes