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.

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

BUG: Regexvalidator option throws error on apply #3691

Closed
1 task done
MiauzGenau opened this issue Aug 11, 2023 · 6 comments
Closed
1 task done

BUG: Regexvalidator option throws error on apply #3691

MiauzGenau opened this issue Aug 11, 2023 · 6 comments
Labels
Bug Label to mark the change as bugfix

Comments

@MiauzGenau
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

If I use the validationErrorMessage option for the Neos.Neos/Validation/RegularExpressionValidator on property, introduced here: #2996.

And then try to apply a change to this property, I get an Unsupported validation option(s) found-error:
Screenshot_2023-06-30_at_16 43 10

Expected Behavior

I would expect to be able to use this option without issues.

Steps To Reproduce

Add the validationErrorMessage option to a RegularExpressionValidator, used as a property validator in the yaml NodeType definition.

Example:

      validation:
        'Neos.Neos/Validation/RegularExpressionValidator':
          regularExpression: /^(?:http(s)?:\/)?(\/)+[\w\-\.~:\/\?#[\]@!\$&\(\)\*\+=]*$/i
          validationErrorMessage: 'Please enter a valid relative or absolute URL.'

Try to apply a change to the property using this validation.
=> Error

Environment

No response

Anything else?

No response

@mhsdesign
Copy link
Member

mhsdesign commented Jan 17, 2024

As the triggering logic is in the ui the issue was moved here.

See my explanation for a good fix:
(originally posted here neos/flow-development-collection#3134 (comment))

But that feels like a bug in Neos, doesn't it? Flow shouldn't allow a property to be changed, because of Neos setting it "accidently", to fire own logic? Maybe we should add an additional validator in Neos then?

yeeeees true. The current situation is suboptimal and flow should not suffer from this. Agreed ^^.
We are currently evaluating better concepts than using flows validators for node properties, so they can be validated in the cr: neos/neos-development-collection#4631

The "auto fire" was introduced via #2351 and actually caused much headache, as the javascript validators and phpvalidators are not always the same and validate differently. This new message feature is just an example.

As this feels indeed quite hacky on php land

If the aim is to change the error message, there are plenty of possibilities. You could use the code and provide an own translation. Or you could override the translation flow provides. Why would you have the message changeable for each validator instance?

i would say the triggering logic in the neos ui php code should be adjusted. As this neos ui property server validation is already super hacky (see #2351) we should fix it there (as bugfix) and keep all the ugly code at one place and when the time comes remove it.

TLTR: I would like to close this in favour of fixing it in the Ui

@mhsdesign
Copy link
Member

Originally posted by @lorenzulrich in neos/flow-development-collection#3134 (comment):

@mhsdesign How would you fix this in the UI? It seems that all validator options are passed to the PHP validator, regardless of whether they are "UI-only" or also valid for the PHP validation.

Wouldn't it make sense to distinguish between the kind of option?

'Vendor.Site:Content.YourNodeTypeName':
  properties:
    lastName:
      type: string
      ui:
		label: 'Last Name'
        inspector:
          group: 'contactData'
      validation:
        'Neos.Neos/Validation/RegularExpressionValidator':
          regularExpression: '...'
          neosUi:
            validationErrorMessage: 'You must set a last name.'

@mhsdesign
Copy link
Member

@lorenzulrich yes that might have made sense when introducing this ^^ But i guess its to late for that discussion, and i would just treat validationErrorMessage as magic ui only string that is used when the validation fails.
That is of course hacky, but as stated previously i think the whole situation is suboptimal to begin with ^^

@lorenzulrich
Copy link
Contributor

I'm note sure if it's too late to fix this. As this currently only can work when the (to be abandoned) Flow patch is applied, it would not be really breaking.

@mhsdesign
Copy link
Member

That is true, but i think this is fine. On the server we are either way only interested in true or false and not about the message so unsetting this option is hacky but straight forward. Eventually this buggy validation concept will hopefully be superseded by neos/neos-development-collection#4631. One day.

@mhsdesign
Copy link
Member

My proposed fix: #3692

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Label to mark the change as bugfix
Projects
None yet
4 participants