search

somnambulist-tech / validation

A re-write of rakit/validation, a standalone validation library inspired by Laravel Validation
MIT License
44 stars 13 forks source link

phpstan & Github actions #3

Closed floriankraemer closed 2 years ago

floriankraemer commented 2 years ago

The readme says it works with PHP 8.0, actually there was a problem with the used phpunit version that I fixed by setting a proper version constraint (the notice was fixed in 9.5.5). composer update --prefer-lowest --prefer-stable revealed that.

I've also made all private calls protected, IMHO methods should never be private in a library.

GH actions will now run for 8.0 and 8.1.

And I renamed the bags to collection. Sorry, but "bag" hurts me. 😄 Feel free to take the change or change it back, but I wanted to share my changes.

dave-redfern commented 2 years ago

Hi @floriankraemer thank you for your PR and interest in contributing to this fork.

Unfortunately I cannot accept your PR as it is too large and makes breaking changes. This lib is actively in use in production projects so any class changes need to be handled with BC in mind.

If you are willing to contribute to this fork of rakit/validation; then I am more than happy to accept your PRs, suggestions, and feedback; but in smaller, more manageable steps.

The first would be if you could isolate the Github action and PHPunit changes into a separate PR, but maintain the additional triggers I have already defined (especially the workflow dispatch - I use this one to re-run the action when it fails due to GH issues), I would be happy to accept those. Incidentally I did not run into any issues myself with PHP 8 or 8.1 so I wonder if this was a transient issue or maybe the GH base image changed?

The second would be the array_is_list fix for PHP 8.0 that I somehow missed - actually I had better patch that now, since it means the lib is broken on PHP 8.0. Likely not run into due to Symfony polyfills.

With regards to some of the other changes you have made:

Bag vs Collection; I kept the previous terminology, but have no strong preference one way or the other and actually did toy with changing it to collection. Symfony uses Bag for ParameterBag, ServerBag etc. Collection is maybe "nicer", but this change would need to be introduced with class_alias for the previous name and appropriate @deprecated notices with removal in the next major version. For reference I maintain my own "collection" project so... 😁

Private vs protected: I take a more conservative approach - especially when it comes to the implementation details and internals of this library. I think there is more benefit of being explicit with what is and is not an intended extension point. Despite my current refactorings; there are still some weird internals that need changing particularly around cloning / re-use of rules, and I think the error message "bag" and messages needs to be redone. So for now, it is safer to keep things private and only allow protected where necessary or it makes sense.

Last point is to do with coding standard. I've not documented it, but many of the changes you made (presumably through phpcs or other automation) are not inline with the current project. I mostly follow PSR 1/2/12 but with some differences where I am more lenient or personally prefer certain constructs (simple inline assignment and compare if (null !== $v = ...) for example I view as being OK). I will find the time to write these up and take a stab at adding phpcs rules, but I've found that I can never quite get these to work exactly how I want.