Make gdpr field required even if the setting is disabled#2282
Conversation
WalkthroughThe changes add GDPR compliance functionality to form field validations. A new filter hook is registered in the controller to trigger GDPR-specific logic. Two methods, Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Controller as FrmHooksController
participant FieldGDPR as FrmFieldGdpr
participant Validator as Form Validator
App->>Controller: load_hooks()
Controller->>FieldGDPR: add_filter('frm_is_field_required', force_required_field)
Validator->>FieldGDPR: force_required_field(required, field)
FieldGDPR-->>Validator: return updated required status
Validator->>FieldGDPR: validate(args)
FieldGDPR-->>Validator: return any validation errors
Possibly related PRs
Suggested labels
✨ Finishing Touches
🪧 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: 2
🧹 Nitpick comments (1)
classes/models/fields/FrmFieldGdpr.php (1)
162-162: Update version number in @SInCE tags.Both new methods use "x.x" as the version number in the @SInCE tags. This should be updated to the actual version number for this release.
Replace "x.x" with the actual version number in both @SInCE tags.
Also applies to: 184-184
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
classes/controllers/FrmHooksController.php(1 hunks)classes/models/fields/FrmFieldGdpr.php(1 hunks)classes/views/frm-fields/back-end/settings.php(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Inspections
classes/models/fields/FrmFieldGdpr.php
[error] 174-174: A function call to __() with texts containing placeholders was found, but was not accompanied by a 'translators:' comment on the line above to clarify the meaning of the placeholders. (WordPress.WP.I18n.MissingTranslatorsComment)
[error] 186-186: Expected 2 spaces after parameter type; 3 found (Squiz.Commenting.FunctionComment.SpacingAfterParamType)
[error] 187-187: Expected 1 spaces after parameter type; 2 found (Squiz.Commenting.FunctionComment.SpacingAfterParamType)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (2)
classes/controllers/FrmHooksController.php (1)
115-117: Appropriate hook added for GDPR field validation.The filter hook for GDPR field validation is properly implemented. This addition ensures that GDPR fields can be forced to be required even when the required setting is disabled.
classes/views/frm-fields/back-end/settings.php (1)
88-88: Improvement to required checkbox behavior for GDPR fields.The checkbox will now be checked when either the field is required OR when it's a readonly required field (like GDPR fields). Additionally, the readonly attribute prevents changing the state when it's a special field type that must be required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
classes/models/fields/FrmFieldGdpr.php (1)
159-180: Ensure the validate method handles localization properly.The validate method correctly enforces GDPR fields as required regardless of the field settings, which aligns with the PR objectives. However, the error message generation on line 175 uses
str_replacewith[field_name]placeholder, which might cause issues for translators.Consider adding a translator comment to clarify the context or switching to a
sprintfapproach with__()for better localization support:- $frm_settings = FrmAppHelper::get_settings(); - $errors[ 'field' . $args['id'] ] = str_replace( '[field_name]', is_object( $this->field ) ? $this->field->name : $this->field['name'], $frm_settings->blank_msg ); + $frm_settings = FrmAppHelper::get_settings(); + $field_name = is_object( $this->field ) ? $this->field->name : $this->field['name']; + /* translators: %s: Field name */ + $errors[ 'field' . $args['id'] ] = sprintf( __( '%s cannot be blank.', 'formidable' ), $field_name );Alternatively, if you prefer to keep using the settings message:
+ // The blank_msg from settings already contains a [field_name] placeholder for translators $frm_settings = FrmAppHelper::get_settings(); $errors[ 'field' . $args['id'] ] = str_replace( '[field_name]', is_object( $this->field ) ? $this->field->name : $this->field['name'], $frm_settings->blank_msg );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/models/fields/FrmFieldGdpr.php(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
classes/models/fields/FrmFieldGdpr.php (4)
classes/models/fields/FrmFieldType.php (1)
validate(1369-1371)classes/helpers/FrmFieldGdprHelper.php (2)
FrmFieldGdprHelper(16-73)hide_gdpr_field(40-43)classes/models/FrmField.php (1)
FrmField(6-1501)classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-4460)get_settings(172-182)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cypress
🔇 Additional comments (2)
classes/models/fields/FrmFieldGdpr.php (2)
182-201: LGTM: force_required_field implementation is correct and follows best practices.The method correctly ensures that GDPR fields are always required, even if the required setting is disabled. This implementation addresses the core issue described in the PR objectives, where GDPR checkboxes could appear uncheckable and unchecked when the setting is disabled.
159-201:Details
✅ Verification successful
Verify integration with the hook controller.
The new methods look good, but we should verify that the
force_required_fieldmethod is properly integrated with the hook system.
🏁 Script executed:
#!/bin/bash # Description: Verify that the force_required_field method is hooked in a controller # Find where the hook for force_required_field is registered rg -A 2 -B 2 "frm_is_field_required.*force_required_field"Length of output: 376
Integration Verified:
force_required_fieldis Properly HookedThe
force_required_fieldmethod is correctly registered in the hook system as confirmed by the output inclasses/controllers/FrmHooksController.php:
- The filter
frm_is_field_requiredis being hooked withFrmFieldGdpr::force_required_fieldusingadd_filter.No further action is needed here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
classes/models/fields/FrmFieldGdpr.php (2)
159-180: Good implementation of validation for GDPR fields, but consider optimizing the error message handling.The validation logic correctly ensures that GDPR fields are required even when the required setting is disabled. The method properly extends the parent validation and only applies additional checks when necessary (when there are no errors and the GDPR field is visible).
However, I suggest improving the error message handling by:
- Using a more descriptive variable name for clarity
- Adding a comment explaining the error message substitution
public function validate( $args ) { $errors = parent::validate( $args ); if ( ! $errors && ! FrmFieldGdprHelper::hide_gdpr_field() ) { $required = FrmField::get_option( $this->field, 'required' ); if ( ! $required && empty( $args['value'] ) ) { $frm_settings = FrmAppHelper::get_settings(); + // Get field name from either object or array format + $field_name = is_object( $this->field ) ? $this->field->name : $this->field['name']; $errors[ 'field' . $args['id'] ] = str_replace( '[field_name]', $field_name, $frm_settings->blank_msg ); } } return $errors; }
182-197: Optimize the force_required_field method with an early return.The method correctly ensures GDPR fields are always required when visible. However, you can optimize it with an early return pattern for improved readability and efficiency.
public static function force_required_field( $required, $field ) { + if ( $required ) { + return $required; + } if ( ! $required && 'gdpr' === $field['type'] && ! FrmFieldGdprHelper::hide_gdpr_field() ) { $required = true; } return $required; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/models/fields/FrmFieldGdpr.php(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
classes/models/fields/FrmFieldGdpr.php (4)
classes/models/fields/FrmFieldType.php (1)
validate(1369-1371)classes/helpers/FrmFieldGdprHelper.php (2)
FrmFieldGdprHelper(16-73)hide_gdpr_field(40-43)classes/models/FrmField.php (1)
FrmField(6-1501)classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-4460)get_settings(172-182)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
🔇 Additional comments (1)
classes/models/fields/FrmFieldGdpr.php (1)
159-197:Details
✅ Verification successful
Verify the integration of both methods in the form submission flow.
Both methods work well together to enforce required status for GDPR fields. The
validatemethod handles validation during form submission, whileforce_required_fieldensures the field is visually marked as required.However, I don't see where
force_required_fieldis being registered as a filter in this file or elsewhere. Make sure it's properly hooked tofrm_is_field_requiredfilter to be effective.
🏁 Script executed:
#!/bin/bash # Check if force_required_field is properly hooked to frm_is_field_required filter rg -A 3 -B 3 "frm_is_field_required.*force_required_field" --type phpLength of output: 522
Integration Verified: GDPR Field Methods
Both methods are properly integrated in the form submission flow. The shell script output confirms that
force_required_fieldis correctly hooked to thefrm_is_field_requiredfilter inclasses/controllers/FrmHooksController.phpwith the following snippet:
add_filter( 'frm_is_field_required', 'FrmFieldGdpr::force_required_field', 10, 2 );No further action is needed.
Liviu-p
left a comment
Liviu-p
left a comment
There was a problem hiding this comment.
@Crabcyborg, it looks good to me. Thank you for fixing it!
|
Thank you Liviu! |
Crabcyborg
deleted the
make_gdpr_field_required_even_if_the_setting_is_disabled
branch
2 participants
Fixes the customer's issue in https://github.com/Strategy11/formidable-pro/issues/5708
It is possible for the GDPR field to not be required. I can at least replicate this by adding a field, disabling the GDPR setting, save the field, and then enabling it again. Since it is read only you end up with a checkbox that is uncheckable and unchecked.
This update always sets the checkbox to appear as checked if the
readonly_requiredsetting is on. When this is on, we expect it to always be checked. I don't see us requiring a readonly disabled checkbox since we would likely just hide the setting in those cases.I also added a check so it always does the required validation even when unchecked, and the required asterisk will still show even when unchecked.
Pre-release
formidable-6.19.1b.zip