-
Notifications
You must be signed in to change notification settings - Fork 28
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
Validation and Custom-Validator throws error since flow 6 #50
Conversation
@skurfuerst hello hello? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey again ☀️
Thank you so much for your contribution! It's awesome to have it ❤️ Sorry for the long delay in answering, and thanks for pinging me on slack to highlight this PR!
I've just gone through the code, and I suggest some adjustments for the following goal:
- I'd love to keep the
RegistrationFlowValidationServiceInterface
as it is right now - as this is part of the public API of the package.
The master
branch of the package is for Flow 6.x; so there's no problem at all to rely on Flow 6 here and fix this behavior directly, without worrying about older Flow releases.
Please ping me again on Slack once you have done some adjustments, as this will help me to prioritize the review in a quick manner 👍
Thanks again for your work,
Sebastian
* @Flow\Inject | ||
* @var TokenAndProviderFactoryInterface | ||
*/ | ||
protected $tokenAndProviderFactoryInterface; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd call this variable $tokenAndProviderFactory
- we usually do not add the interface
postfix on variable names :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
if ($this->objectManager->isRegistered(RegistrationFlow6ValidationServiceInterface::class)) { | ||
$instance = $this->objectManager->get(RegistrationFlow6ValidationServiceInterface::class); | ||
$instance->validateRegistrationFlow6($value, $this->getResult()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(read comment about getResult first)
... then, you can skip this block, because the RegistrationFlowValidationServiceInterface will work as usual before :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wesentlich eleganter, klar
README.md
Outdated
// This is an example of your own custom validation logic. | ||
if ($registrationFlow->getAttributes()['agb'] !== '1') { | ||
$validator->getResult()->forProperty('attributes.terms')->addError(new \Neos\Flow\Validation\Error('You need to accept the terms and conditions.')); | ||
$validatorResult->forProperty('attributes.terms')->addError(new \Neos\Flow\Validation\Error('You need to accept the terms and conditions.')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... then these changes here are obsolete :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Habe die README rückgängig gemacht.
@skurfuerst Besten Dank! Habe die Änderungen einmal vorgenommen. Habe es in meinem Projekt getestet - funktionieren gut 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey,
thanks, looks really good :) Just one more tiny detail (see review).
All the best,
Sebastian
/** | ||
* @api | ||
*/ | ||
interface RegistrationFlow6ValidationServiceInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you still remove the one obsolete interface? :-) :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch
thanks, looks great :) Merging! |
@skurfuerst
Hi there,
this is my first pull request - please be gentle :)
Cheers
Göks