Phalcon5 migration: provide compatibility layer between v4 and v5 #5711
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…alidation classes. Since Validation() in 5 moves to Filter\Validation (https://docs.phalcon.io/5.0/en/upgrade#general-notes) and the class is used in strict parameter passing, it will be difficult to import a situation which works both on 4 and 5. To prevent future issues, but keep the current situation functional with minimal changes, we wrapped Validation() into a class of our own. When validators inherit from BaseValidator, these are assumed to be less strict and bound to our own handling. Phalcon validations will follow the old path for now. Migrating existing validations on our end, should be as simple as changing the following lines: -use Phalcon\Validation\AbstractValidator; -use Phalcon\Validation\ValidatorInterface; -use Phalcon\Validation; +use OPNsense\Base\BaseValidator; -class CallbackValidator extends AbstractValidator implements ValidatorInterface +class CallbackValidator extends BaseValidator - public function validate(Validation $validator, $attribute): bool + public function validate($validator, $attribute): bool Todo: choose the "correct" Validation() based on phalcon version.
uses bc881b9 to remove the need for AbstractValidator and Validator(Interface). This commit changes the relevant includes and provides some changes to make the unit tests run correctly.
…alidation classes. part duex Although our previous strategy should work according to how bind() and validation() are being implemented (https://github.com/phalcon/cphalcon/blob/4.2.x/phalcon/Validation.zep), in reality it seems they aren't the same. Our previous attempt failed some validations (such as booleans) for no valid reasons. Long term we should remove the phalcon dependency as these effects are highly unpredictable. phalcon5: update Validation class to now pick the right Phalcon Validation based on version
AdSchellevis
added a commit
that referenced
this pull request
Apr 17, 2022
…alidation classes. Minor regression in previous commit, performValidation()'s return type didn't match. Ideally we would rather switch to plain array's, but since performValidation() is used in multiple areas (including plugins) we better opt for compatiblity now. ref #5711
…alidation classes. Minor regression in previous commit, performValidation()'s return type didn't match. Ideally we would rather switch to plain array's, but since performValidation() is used in multiple areas (including plugins) we better opt for compatiblity now. ref #5711
…t tests as needed See 6814f32 as to why this is the case.
|
@AdSchellevis Thanks, adjusted the PR to account for this change. Unit tests cannot expect |
fichtner
reviewed
Apr 19, 2022
Co-authored-by: Franco Fichtner <[email protected]>
fichtner
pushed a commit
that referenced
this pull request
May 9, 2022
) * phalcon5: update namespaces * phalcon5 Autoload: rename registerDirs() to setDirectories() * phalcon5: remove trailing slash * phalcon5: default to php74-phalcon * phalcon5: provide 4/5 compatibility layer except for AbstractValidator * MVC - Phalcon 5 migration and options to lose dependency of phalcon validation classes. Since Validation() in 5 moves to Filter\Validation (https://docs.phalcon.io/5.0/en/upgrade#general-notes) and the class is used in strict parameter passing, it will be difficult to import a situation which works both on 4 and 5. To prevent future issues, but keep the current situation functional with minimal changes, we wrapped Validation() into a class of our own. When validators inherit from BaseValidator, these are assumed to be less strict and bound to our own handling. Phalcon validations will follow the old path for now. Migrating existing validations on our end, should be as simple as changing the following lines: -use Phalcon\Validation\AbstractValidator; -use Phalcon\Validation\ValidatorInterface; -use Phalcon\Validation; +use OPNsense\Base\BaseValidator; -class CallbackValidator extends AbstractValidator implements ValidatorInterface +class CallbackValidator extends BaseValidator - public function validate(Validation $validator, $attribute): bool + public function validate($validator, $attribute): bool Todo: choose the "correct" Validation() based on phalcon version. * phalcon5: complete migration to compatibility layer. uses bc881b9 to remove the need for AbstractValidator and Validator(Interface). This commit changes the relevant includes and provides some changes to make the unit tests run correctly. * MVC - Phalcon 5 migration and options to lose dependency of phalcon validation classes. part duex Although our previous strategy should work according to how bind() and validation() are being implemented (https://github.com/phalcon/cphalcon/blob/4.2.x/phalcon/Validation.zep), in reality it seems they aren't the same. Our previous attempt failed some validations (such as booleans) for no valid reasons. Long term we should remove the phalcon dependency as these effects are highly unpredictable. phalcon5: update Validation class to now pick the right Phalcon Validation based on version * MVC - Phalcon 5 migration and options to lose dependency of phalcon validation classes. Minor regression in previous commit, performValidation()'s return type didn't match. Ideally we would rather switch to plain array's, but since performValidation() is used in multiple areas (including plugins) we better opt for compatiblity now. ref #5711 * phalcon5: switch back to using count() on Messages object, adjust unit tests as needed See 6814f32 as to why this is the case. * phalcon5: remove PhalconGenerator and references, use checked-in files instead * Update Makefile Co-authored-by: Franco Fichtner <[email protected]> Co-authored-by: Ad Schellevis <[email protected]> Co-authored-by: Franco Fichtner <[email protected]> (cherry picked from commit cfd4b76)
AdSchellevis
added a commit
to opnsense/ui_devtools
that referenced
this pull request
May 30, 2022
AdSchellevis
added a commit
to opnsense/ui_devtools
that referenced
this pull request
Jun 1, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR does the following:
Provide php code to generate classes that provide most of the heavy lifintg for operating between the namespace changes in v4 -> v5.The generated classes are included for check-in here as well to fuel the discussion for whether we should dynamically generate these or just provide the from the source repository. Main argument is to keep the diff reduced, but generated code might be fragile to corruption.The new Validator() class does not return a Messages() object, but an array. This means Messages()->Count() occurrences do not work anymore. i've changed these to use a normal array count.Following the same logic, the expectations of certain unit tests have changed as well.All of this should be thoroughly tested before merging it into master.