Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[5.3] Rework form validation #42752

Draft
wants to merge 16 commits into
base: 5.3-dev
Choose a base branch
from
Draft

[5.3] Rework form validation #42752

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
9bd1d26
First pass at form validation rework
wilsonge Dec 28, 2023
20e5d53
Merge branch '5.1-dev' into feature/form-validation-rework
wilsonge Feb 3, 2024
8a78203
Update libraries/src/Form/Constraint/FieldLengthConstraint.php
wilsonge Feb 4, 2024
40c2af3
Update max length comment
wilsonge Feb 4, 2024
5746f80
Fix process method for the results of validate
wilsonge Feb 25, 2024
cbcb4fa
Inject group when getting the field
wilsonge Feb 25, 2024
57781d3
More changes for the new method
wilsonge Feb 25, 2024
6e68dd4
Merge remote-tracking branch 'origin/5.1-dev' into feature/form-valid…
wilsonge Feb 25, 2024
f2b6e58
Fix return type check
wilsonge Feb 25, 2024
e381bf8
Allow field length value to be null
wilsonge Feb 25, 2024
f6b5cb9
Merge remote-tracking branch 'origin/5.1-dev' into feature/form-valid…
wilsonge Feb 25, 2024
8597da1
PHPCS
wilsonge Feb 25, 2024
ebc97e7
Fix group concept
wilsonge Feb 25, 2024
26ba333
Merge branch '5.1-dev' into feature/form-validation-rework
wilsonge Apr 14, 2024
2c82b7b
Merge branch '5.3-dev' into feature/form-validation-rework
richard67 Sep 15, 2024
6379f5c
Fix PHPCS
richard67 Sep 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Fix PHPCS
  • Loading branch information
@richard67
richard67 committed Sep 15, 2024
commit 6379f5cf93b500aef35508e56d066fad1c2e8c92
2 changes: 1 addition & 1 deletion libraries/src/Form/Constraint/FieldLengthConstraint.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public function __construct(FormField $field, ?string $value, int $maxLength)
public function getErrorMessage(): string
{
if ($this->isValid()) {
throw new \BadMethodCallException(sprintf('Field %s is valid', $this->getField()->getAttribute('name', '')));
throw new \BadMethodCallException(\sprintf('Field %s is valid', $this->getField()->getAttribute('name', '')));
}

return Text::sprintf('JLIB_FORM_VALIDATE_FIELD_INVALID', $this->getFieldLabel());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a string specific to the field shouldnt it? So in this case it woujld be JLIB_FORM_VALIDATE_FIELD_FIELDLENGTH_INVALID

Copy link
Contributor Author

@wilsonge wilsonge Feb 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now I left the errors as they are. Because most this validation is done client side in many cases (maybe API excepted?) end users shouldn't see this unless they are doing malicious things - so I decided to keep the errors very generic. Technically this is a new error (unlike the other comment below - so I'm happy either way)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By making it specific it would be easier (i think) to have specific error messages with advice on how to resolve the problem. See #41140 for a real world use case

Expand Down
2 changes: 1 addition & 1 deletion libraries/src/Form/Constraint/FieldRequiredConstraint.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class FieldRequiredConstraint extends AbstractConstraint
public function getErrorMessage(): string
{
if ($this->isValid()) {
throw new \BadMethodCallException(sprintf('Field %s is valid', $this->getField()->getAttribute('name', '')));
throw new \BadMethodCallException(\sprintf('Field %s is valid', $this->getField()->getAttribute('name', '')));
}

return Text::sprintf('JLIB_FORM_VALIDATE_FIELD_REQUIRED', $this->getFieldLabel());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a string specific to the field shouldnt it? So in this case it woujld be JLIB_FORM_VALIDATE_FIELD_REQUIRED_INVALID

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same as the current value https://github.com/joomla/joomla-cms/blob/4.4-dev/libraries/src/Form/FormField.php#L1170-L1175

Expand Down
2 changes: 1 addition & 1 deletion libraries/src/Form/Constraint/LegacyRuleConstraint.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public function getRule(): FormRule
public function getErrorMessage(): string
{
if ($this->isValid()) {
throw new \BadMethodCallException(sprintf('Field %s is valid', $this->getField()->getAttribute('name', '')));
throw new \BadMethodCallException(\sprintf('Field %s is valid', $this->getField()->getAttribute('name', '')));
}

// Does the field have a defined error message?
Expand Down
2 changes: 1 addition & 1 deletion libraries/src/Form/Field/TextField.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ protected function getLayoutData()
* @throws \UnexpectedValueException When the field or a rule configuration is invalid
*@since 4.0.0
*/
public function validate($value, $group = null, Registry $input = null)
public function validate($value, $group = null, ?Registry $input = null)
{
$validationResults = parent::validate($value, $group, $input);

Expand Down
10 changes: 5 additions & 5 deletions libraries/src/Form/Form.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ public function getErrors()
{
if ($this->modernValidationResponse === true) {
throw new \BadMethodCallException(
sprintf(
\sprintf(
'%1s should not be used when %2s::modernValidationResponse is enabled',
__METHOD__,
static::class
Expand Down Expand Up @@ -1136,7 +1136,7 @@ public function validate($data, $group = null)
if (!$fields) {
// PANIC!
if ($this->modernValidationResponse) {
throw new \InvalidArgumentException(sprintf('There were no fields to validate for group %s', $group));
throw new \InvalidArgumentException(\sprintf('There were no fields to validate for group %s', $group));
}

return false;
Expand Down Expand Up @@ -1183,15 +1183,15 @@ public function validate($data, $group = null)

if ($fieldValidationResponse instanceof \Exception) {
$validationResponse->addField(new LegacyInvalidField($name, $group, $fieldLabel, $fieldValidationResponse));
@trigger_error(sprintf('From 7.0 fields must return a class implementing %s.', FieldValidationResponseInterface::class), E_USER_DEPRECATED);
@trigger_error(\sprintf('From 7.0 fields must return a class implementing %s.', FieldValidationResponseInterface::class), E_USER_DEPRECATED);
} elseif ($fieldValidationResponse === true) {
$validationResponse->addField(new LegacyValidField($name, $group, $fieldLabel));
@trigger_error(sprintf('From 7.0 fields must return a class implementing %s.', FieldValidationResponseInterface::class), E_USER_DEPRECATED);
@trigger_error(\sprintf('From 7.0 fields must return a class implementing %s.', FieldValidationResponseInterface::class), E_USER_DEPRECATED);
} elseif ($fieldValidationResponse instanceof FieldValidationResponseInterface) {
$validationResponse->addField($fieldValidationResponse);
} else {
throw new \UnexpectedValueException(
sprintf(
\sprintf(
'Unexpected response from %s::validate, received %s',
$fieldObj::class,
$fieldValidationResponse::class
Expand Down
2 changes: 1 addition & 1 deletion libraries/src/Form/FormField.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1223,7 +1223,7 @@ public function validate($value, $group = null, ?Registry $input = null)
$response->addConstraint($rule);
} else {
@trigger_error(
sprintf('Rules should implement %s from 6.0.', FormRuleInterface::class),
\sprintf('Rules should implement %s from 6.0.', FormRuleInterface::class),
E_USER_DEPRECATED
);

Expand Down
4 changes: 2 additions & 2 deletions libraries/src/Form/Rule/AbstractRegexRule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,13 @@ abstract class AbstractRegexRule implements FormRuleInterface
* @since 1.6
* @throws \UnexpectedValueException If regex is invalid.
*/
public function test(\SimpleXMLElement $element, mixed $value, string $group = null, Registry $input = null, Form $form = null): void
public function test(\SimpleXMLElement $element, mixed $value, ?string $group = null, ?Registry $input = null, ?Form $form = null): void
{
$this->ruleRun = true;

// Check for a valid regex.
if (empty($this->regex)) {
throw new \UnexpectedValueException(sprintf('%s has invalid regex.', \get_class($this)));
throw new \UnexpectedValueException(\sprintf('%s has invalid regex.', \get_class($this)));
}

// Detect if we have full UTF-8 and unicode PCRE support.
Expand Down
2 changes: 1 addition & 1 deletion libraries/src/Form/Rule/FormRuleInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,5 +40,5 @@ interface FormRuleInterface extends ConstraintInterface
* @since __DEPLOY_VERSION__
* @throws \UnexpectedValueException if rule is invalid.
*/
public function test(\SimpleXMLElement $element, $value, $group = null, Registry $input = null, Form $form = null): void;
public function test(\SimpleXMLElement $element, $value, $group = null, ?Registry $input = null, ?Form $form = null): void;
}
2 changes: 1 addition & 1 deletion libraries/src/Form/Rule/RuleConstraintTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public function isValid(): bool
public function getErrorMessage(): string
{
if (!$this->ruleRun) {
throw new \BadMethodCallException(sprintf('The %s::test() method must be run', static::class));
throw new \BadMethodCallException(\sprintf('The %s::test() method must be run', static::class));
}

if ($this->isValid) {
Expand Down
2 changes: 1 addition & 1 deletion libraries/src/Form/Validation/Field/LegacyValidField.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ public function getName(): string
*/
public function getErrorMessage(): string
{
throw new \BadMethodCallException(sprintf('Field %s is valid', $this->fieldLabel));
throw new \BadMethodCallException(\sprintf('Field %s is valid', $this->fieldLabel));
}
};
}
Expand Down