Project

General

Profile

Actions

Bug #104118

open

Avoid hard dependency on final validation viewhelper

Added by Andreas Kießling about 1 month ago. Updated about 1 month ago.

Status:
Needs Feedback
Priority:
Should have
Assignee:
-
Category:
Form Framework
Target version:
-
Start date:
2024-06-15
Due date:
% Done:

0%

Estimated time:
TYPO3 Version:
12
PHP Version:
Tags:
Complexity:
medium
Is Regression:
Sprint Focus:

Description

I inherited a project that xclassed the NotEmptyViewHelper and am currently updating it to v12.4
Previously, the ViewHelper was extended, but since they are final now this can't be done anymore.
Thus i copied the code over, made the adjustments --> done, or so i thought...

Now when i tried to add a new form with EXT:form, i could not get the required sign to show up. After quite some debugging i found that there are hard dependencies with instanceof checks in place
https://github.com/search?q=repo%3ATYPO3%2Ftypo3+%22instanceof+NotEmpty%22&type=code

https://github.com/TYPO3/typo3/blob/main/typo3/sysext/extbase/Classes/Validation/Validator/NotEmptyValidator.php

IMHO this implementation needs to be changed, we can not have any hardcoded checks that way without being able to switch out the code. I know that the final declaration is unlikely to change, thus the NotEmptyValidator should probably have an interface or tag to indicate it is a required validator.


Related issues 2 (0 open2 closed)

Related to TYPO3 Core - Bug #101927: TYPO3 Fluid Core ViewHelpers should not be declared "final class"Rejected2023-09-15

Actions
Related to TYPO3 Core - Bug #100196: Remove "final" on public (!) core viewhelpersRejected2023-03-17

Actions
Actions #1

Updated by Andreas Kießling about 1 month ago

  • Related to Bug #101927: TYPO3 Fluid Core ViewHelpers should not be declared "final class" added
Actions #2

Updated by Andreas Kießling about 1 month ago

  • Related to Bug #100196: Remove "final" on public (!) core viewhelpers added
Actions #3

Updated by Garvin Hicking about 1 month ago

  • Status changed from New to Needs Feedback

I would like to understand the implementation needs and workflow a bit better. Could you maybe describe what the custom ViewHelper does differently?

Maybe there could be some solution by using a distinct custom validator which is also used by the custom viewhelper? Maybe basing some parts on an interface can be done (instead of instanceof), and wouldn't break the contract between the core and the Validator&ViewHelper (&extbase) for final classes.

(I'm not mentioning composer patches here of course....)

Actions #4

Updated by Andreas Kießling about 1 month ago

The "empty" check in the NotEmpty viewhelper is a bit flawed since it also accepts empty strings.

Thus the value is trimmed in the xclassed viewhelper before checking

public function isValid(mixed $value): void
    {
        $value = \is_string($value) ? trim($value) : $value;

The $acceptsEmptyValues flag is set, but the isEmpty check checks for null and empty strings

final protected function isEmpty(mixed $value): bool
    {
        return $value === null || $value === '';
    }

Maybe there could be some solution by using a distinct custom validator which is also used by the custom viewhelper?

I did not really get the meaning of this. Maybe you mean i add the core NotEmpty and a custom validation? Would take quite some code changes and testing again... And whenever a new form is added, this has to be remembered.

Maybe basing some parts on an interface can be done (instead of instanceof), and wouldn't break the contract between the core and the Validator&ViewHelper (&extbase) for final classes.

That's what i was thinking of, or an annotation to indicate that this validation means required, or maybe call a method on the the viewhelper to let it decide if it it means i am required

I'd also e.g. consider a StringLength Validator to mean "required" for a form if minLength > 0, but currently you have to explicitly set Required in the form module, meaning it adds a NotEmpty validator in the yaml.
The StringLength Validator also does not set acceptsEmptyValues to false, thus it HAS to be combined with NotEmpty

--> The wording and functionality is pretty confusing right now to get it right.

(I'm not mentioning composer patches here of course....)

Well, that's what i will go for since i don't want to patch all the custom code and EXT:form to get a consistent behavior.

Actions

Also available in: Atom PDF