zendframework / zend-validator

Validator component from Zend Framework
BSD 3-Clause "New" or "Revised" License
181 stars 136 forks source link

Road to ZF3 #1

Open bakura10 opened 9 years ago

bakura10 commented 9 years ago

Hi,

I've done a massive PR to this (https://github.com/zendframework/zf2/pull/5067) but it's a massive work and a big hell to review.

I suggest that we:

  1. Move validators to their specifc components (for instance moving Db validators to the Zend\Db component), and deprecating the validators here (for version 2.5).
  2. Remove them in v3, and start the refactoring work. Here are various things I have done in the past:
    • Make validators stateless (each validator returns a ValidationResult, that can be translated using another component for instance).
    • Using the new service manager of ZF3.
    • Homogeneize names (in ZF2, we didn't have time to use the "underscore_separated" convention for option name, which make it very incoherent with how the framework work).
    • Various code cleaning, optimization for PHP 5.5
snapshotpl commented 9 years ago

:+1: for stateless

Maks3w commented 9 years ago

I suggest to split the proposal in invidual issues for to have clean discussion threads.

:+1: For place validator inside of each component.

weierophinney commented 9 years ago

Move validators to their specifc components (for instance moving Db validators to the Zend\Db component), and deprecating the validators here (for version 2.5).

First, v2.5 is imminent; I just need to ensure all the components are passing tests on current master, and then I'll be tagging their 2.5 versions, followed by the refactoring of the ZF2 repo to be a metapackage (see zendframework/zf2#7542 and my own tree that extends that). As such, deprecation would have to be following 2.5.

That said, I'm a bit leery of spreading the validators amongst all the various component repositories:

The second can be solved, of course, by modifying the current "suggest" section of the composer.json to refer to the validators instead of the dependency; however, it still provides overhead in consuming the validators for the end-user, as they both need to install the additional package(s) and then configure the validators in the plugin manager.

We've seen already that having validators split between zend-i18n and zend-validator causes problems when folks are looking for common validators such as alpha, alnum, DateTime, IsFloat, and IsInt; they expect them to be in zend-validator, and have no idea that they might be elsewhere.

akrabat commented 9 years ago

Also, as someone who uses Zend\Validator standalone, I don't want to bring in the whole world. If anything I'd like basic i18n ones to be in Zend\Validator.

bakura10 commented 9 years ago

I'm definitely not against bringing back validators to this package. But on the other hand it's just moving the problem : instead of introducing a validator dependency to crypt or db, we introduce a dependency to crypt and db in validator. Not sure which one is better.

Envoyé de mon iPhone

Le 28 mai 2015 à 21:43, Rob Allen notifications@github.com a écrit :

Also, as someone who uses Zend\Validator standalone, I don't want to bring in the whole world. If anything I'd like basic i18n ones to be in Zend\Validator.

— Reply to this email directly or view it on GitHub.

weierophinney commented 9 years ago

instead of introducing a validator dependency to crypt or db, we introduce a dependency to crypt and db in validator

Not the way we have it currently. The only hard dependency is zend-stdlib at the moment. We have 8 "suggest" components, each indicating they should be installed if you need to use specific validators. (Those same are added as dev dependencies in order to test.)

In production, you use composer install --no-dev, which means you'll not get any extra dependencies.

bakura10 commented 9 years ago

So whats the difference if we move the validators to their own components? :D I definitely agree that i18n should be in the validators package because they actually don't rely on any class in i18n package.

But for db and crypt they definitely should be in Zend/db and Zend/crypt as they rely on those. No?

Envoyé de mon iPhone

Le 28 mai 2015 à 22:12, weierophinney notifications@github.com a écrit :

instead of introducing a validator dependency to crypt or db, we introduce a dependency to crypt and db in validator

Not the way we have it currently. The only hard dependency is zend-stdlib at the moment. We have 8 "suggest" components, each indicating they should be installed if you need to use specific validators. (Those same are added as dev dependencies in order to test.)

In production, you use composer install --no-dev, which means you'll not get any extra dependencies.

— Reply to this email directly or view it on GitHub.

manuakasam commented 9 years ago

My only argument would be to choose one over the other. Like @bakura10 said it's somewhat of moving the problem from A to B. There's really no difference between those two scenarios:

The result is the same. In both scenarios the dependency is a suggest only.

It IS important though, that we choose one of the two. The way it is currently handled, some validators aren't within the zend-validator package. And truthfully, no one knows this until the ServiceManager throws the error that the validator can't be found ;) Not even myself. Quite frankly, I don't give a damn as to what packages are "suggested" towards me by composer. Suggestions don't automatically imply "requirements" if I want to use feature X or Y. And my strong guess is that I am not the only one having this interpretation of suggested packages.

Personally my vote would likely go to have all validators within zend-validator. Given the package name this feels more intuitive. And it would likely be less painful in teaching the beginner end of developers in how to be using them.

Michaël Gallego notifications@github.com schrieb am Fr., 29. Mai 2015 um 00:04 Uhr:

So whats the difference if we move the validators to their own components? :D I definitely agree that i18n should be in the validators package because they actually don't rely on any class in i18n package.

But for db and crypt they definitely should be in Zend/db and Zend/crypt as they rely on those. No?

Envoyé de mon iPhone

Le 28 mai 2015 à 22:12, weierophinney notifications@github.com a écrit :

instead of introducing a validator dependency to crypt or db, we introduce a dependency to crypt and db in validator

Not the way we have it currently. The only hard dependency is zend-stdlib at the moment. We have 8 "suggest" components, each indicating they should be installed if you need to use specific validators. (Those same are added as dev dependencies in order to test.)

In production, you use composer install --no-dev, which means you'll not get any extra dependencies.

— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHub https://github.com/zendframework/zend-validator/issues/1#issuecomment-106613510 .

froschdesign commented 9 years ago

Only a screenshot: zend-validator :-1:

Maks3w commented 9 years ago

@bakura10 Could you open a different issue for each different idea? This will allow to have different discussion threads about the same thing (I hope)

By the way please reword this issue for only discuss the validator split thing.

Maks3w commented 9 years ago

I think we mix two different scopes sometimes. The framework scope and the library scope.

As framework we want to have a single point where to list and discover available validators. As library we want to be decoupled and provide the basis so other developers implement his/her custom validators.

This is my list of benefits of split validators in different components:

This is the list of problems if validators are split:

From my point of view the "problems" described must be resolved by the framework and not by the component at may reveal the same issues a developer has when create a custom component and expect to be used in a zendframework project.

Possible alternative solutions:

bakura10 commented 9 years ago

Foo-Validators are more coupled with Foo releases so component and subcomponent can be released at the same time without to wait for a Zend\Validator release

This is definitely the point that I didn't think about that definitely should push us to separate validators into separate repositories. Otherwise we are going to have the same problem as what we had in ZF2 (although at a smaller scope).

This would mean that if we update Zend\Crypt to v3 for instance (with a BC change somewhere) we'll be force to make Zend\Validator v3 also because of the dependency.

akrabat commented 9 years ago

Hmm.. yeah I see that point, too.

weierophinney commented 9 years ago

So whats the difference if we move the validators to their own components?

In a word: Discovery.

Most of the assumptions I've seen in this thread are that people are using the component in the context of Zend Framework. One of the points of splitting the components into their own repositories, however, is to emphasize that ZF is a component library foundation, on which a framework is built. This emphasis is to encourage developers to use ZF components even if they are not using the framework.

If you come to zend-validator from that direction, would you ever consider looking in zend-db for validators? or zend-i18n? or zend-crypt? Chances are that you would have no idea that those packages also deliver validators, and, when you are unable to find the validator you need in zend-validator, you move on to a different library entirely.

That's the fundamental argument I've been trying to make.

Foo-Validators are more coupled with Foo releases so component and subcomponent can be released at the same time without to wait for a Zend\Validator release This is definitely the point that I didn't think about that definitely should push us to separate validators into separate repositories. Otherwise we are going to have the same problem as what we had in ZF2 (although at a smaller scope).

This would mean that if we update Zend\Crypt to v3 for instance (with a BC change somewhere) we'll be force to make Zend\Validator v3 also because of the dependency.

Untrue. zend-validator will suggest the components on which it depends, with the versions it is tested against. The require-dev section will reference the major.minor pair for semantic versioning as well, meaning you can easily determine that zend-validator is still only supporting the v2 versions of zend-crypt.

We can, in a minor release of zend-validator up the version requirement for these specific validators.

As an example, let's say zend-validator is currently on v2.6, and suggests ~2.5 for zend-crypt if you are using the crypt-specific validators. We then release a v3 of zend-crypt. If you are using zend-validator, and require zend-crypt, you are still pulling in a v2 series zend-crypt, as zend-validator does not support v3 of zend-crypt yet. For v2.7, we make the necessary changes to support zend-crypt v3, and suggest ~3.0 for zend-crypt; a user who updates their application will now have zend-validator ~2.7 and zend-crypt at ~3.0.

The only place this will be a problem is if elsewhere in their application they want/need zend-crypt v3 while they're still on ~2.6 of zend-validator. In that particular case, the developer can always opt to write their own implementation(s) of the zend-crypt-based validator(s).

We have two paths, then:

Both have pros and cons, and lead to issues for developers.

Personally, with Apigility, I forgot that we had validators elsewhere when we wrote the zf-content-validation module. If we consider that even veteran ZF users can miss/forget this fact, what hope do new users have of finding them? As such, I recommend strongly that we keep all validators and all filters in the same packages.

weierophinney commented 9 years ago

With regards to the other points, I'm quite happy with this pitch. :+1:

In particular, making them stateless, and having a "validation result" object makes a ton of sense, and promotes far better re-usability. Considering that most ZF developers interact with validators primarily through InputFilters (or Forms, but Forms just delegate to InputFilters), the change will be essentially transparent for most developers as well.

Maks3w commented 9 years ago

Personally, with Apigility, I forgot that we had validators elsewhere when we wrote the zf-content-validation module. If we consider that even veteran ZF users can miss/forget this fact, what hope do new users have of finding them? As such, I recommend strongly that we keep all validators and all filters in the same packages.

This is because the discovery problem exists, will continue to exist and won't be fixed ever.

For me put all validators in the same repo is not an option.

Apigility is expected to have his own validators and them are not expected to be included in this repo because may to be too specific even while them could be reused.

My suggest is:

If not possible then:

As I said in the above posts. Discover is not an issue of the component, it's an issue of the Framework and should be fixed at Framework level (providing an index in ZF2 repo)

This would mean that if we update Zend\Crypt to v3 for instance (with a BC change somewhere) we'll be force to make Zend\Validator v3 also because of the dependency.

Untrue. zend-validator will suggest the components on which it depends, with the versions it is tested against. The require-dev section will reference the major.minor pair for semantic versioning as well, meaning you can easily determine that zend-validator is still only supporting the v2 versions of zend-crypt.

We can, in a minor release of zend-validator up the version requirement for these specific validators.

  1. This is a discussion about the global release process and this thread is not the best place for to discuss them.
  2. I think in the following scenario:
Root: {
 Foo: ^1,
 Baz: ^1: {
     Foo: ^1 // Same dependency as root package.
}

What happens if Baz publish a security patch 1.0.1 and raise his dependency to Foo: ^2

Root: {
 Foo: ^1,
 Baz: ^1: {
     Foo: ^2 // Version conflicts with root constraints.
}

This means the security patch (and any other non BC release) cannot be deployed without the developer raise his dependency to Foo: ^2

For me BC releases implies anyone can be trust in to have the last non BC version without to change any line of code.

gianarb commented 9 years ago

@bakura10 are you sure that point A is a good idea? in my opinion we can move db validators into the different project es. zend-db-validator-interop.. :) or similar..

bakura10 commented 9 years ago

Promote ValidatorPluginManager to ZF2 repo with the whole list of validators maintained by the project.

This is the thing I strongly disagree. As I said, we NEED to find a way to aggregate config for non-modules. As said several times, we should either have a concept of "mini-module" for component framework, or make each component a module, and allow syntax such as:

return [
   'modules' => [
      'Zend\*'
   ]
];

Of course, this implies that we refactor a bit the module manager to make it more lightweight, or having more aggressive caching. But I feel very bad to install a some sort of meta package just for populating the various plugin managers (as it iwll also means that any addition to any package will require us to also update this meta package).

(PS : and yes, I agree with splitting, Zend\Db validators should not belong to this package, they are to me the same concept as a Aws validator inside an Aws module, it should definitely sits in another module). Zend\Validator should focus on the basic architecture and provide dependency-free vlaidators

Maks3w commented 9 years ago

@bakura10 I don't see your argument. Are you proposing some kind of autodiscover or selfregister protocol?

bakura10 commented 9 years ago

Exactly. Currently to do all the wiring up inside the framework, we are using this: https://github.com/zendframework/zend-mvc/blob/master/src/Service/ServiceListenerFactory.php which is ugly because of all the hidden dependencies.

The kind of meta package you are suggesting is I think a wrong idea because it will introduce back a single package that has dependencies over each system that use a plugin manager, to wire the various plugins of each component and goes against the new philosophy of components.

I've already thought of various experiments to make each component registering their validators/filters/hydrators/whatever into the plugin manager, but I think the simplest soltuion is simply to introducing a concept of "light module" (but the problem is that it will be a bit consistent... why only the official components would use that?) or going to the logic : component become module.

But definitely, a solution needs to be found in this area :)

Maks3w commented 9 years ago

Do we all agree discover feature is outside of the scope of the Validator component itself?

bakura10 commented 9 years ago

Not completely, as it's a piece to making the splitting of validators/filters more easier. Ideally, people should be able to install Zend\Validator, then Zend\Db, and being able to have Zend\Db validators available in the plugin manager without having to wiring up more things.

Maks3w commented 9 years ago

My point is zend-validator should have only code about definition and common tools for validation.

All those stuff about "integration", "discover" or whatever you want to name is out of the library and should be managed outside.

Zend\Validator is good as a standalone project. Wrappers about how to plug into ZF2 services should be done outside, same as if someone want to wrap the library to a SF bundle or AnotherServiceContainerProject

Outside means:

Finally, the roadmap shouln't be blocked pending about to design the autoregister protocol proposed by bakura. If we agree with the rest with the points we should start to work on them and manage a static index of validators (outside of this repo) until a best option is present.

bakura10 commented 9 years ago

So you bascially suggest that we actually completely remove the ValidatorPluginManager from this library then? (otherwise your point does not make sense ^^).

Maks3w commented 9 years ago

yep

gianarb commented 9 years ago

So you bascially suggest that we actually completely remove the ValidatorPluginManager from this library then? (otherwise your point does not make sense ^^).

Today is a very important day! We have ideally removed two dependencies:

from Zend\Validator! wow :+1:

fntlnz commented 9 years ago

This is pretty awesome! :tada:

bakura10 commented 9 years ago

Ok !

I will therefore try to work on the refactor (based on my previous work and those new feedbacks!) asap :).

weierophinney commented 9 years ago

Before you go too far on this, I need to weigh in here.

I thoroughly disagree with both proposals: both moving component-specific validators into their components, and removing the plugin manager.

The arguments made so far center primarily around one concept: separation of concerns. Removal of component-specific validators means that the zend-validator component is both slimmer code-wise, as well as dependency-wise. Removal of the plugin manager means losing a component. In both cases, the idea is to push this stuff elsewhere: component-specific validators go into their components, and things like plugin management become a "framework concern."

My argument against the change is similarly simple to state: it puts too much onus on the consumer.

@Maks3w has alluded to one aspect already: discoverability of validators. He maintains that discoverability should be pushed into the framework, or into framework-specific packages (e.g., ZF2 modules, Symfony bundles, etc.). I think that's a cop-out: it means that if you come across the zend-validator component, it's not immediately usable. You have to either do further discovery to find the correct consumable package, or you end up having to figure out how to wire it yourself. Either way, you're left with something you cannot use easily and simply.

Second, it pushes the dependency problem elsewhere. Right now, any validator that has additional dependencies leads to a new suggest line in composer.json, which, if they were properly worded, would read something like:

"zendframework/zend-db": "~2.0 if you wish to use the various DB-centric validators"

These same dependencies are mirrored in our require-dev section to enable testing.

If we move the validators to their own components, we get multiple new problems:

Next, let's look at removal of the plugin manager.

One key usage of validators within zend-validator is via ValidatorChain. This can be used by specifying either validator instances, or validator names; when the latter are used, we pull from a plugin manager. This makes for simpler usage:

$chain->attachByName('IsInt')
    ->attachByName('Between', ['min' => 100, 'max' => 1000']);

This could just as easily be done with instances, obviously:

$chain->attach(new IsInt())
    ->attach(new Between(100, 1000));

However, the most common way to use validators is via a configuration-based tool such as zend-inputfilter or zend-form. In those cases, you're using something along these lines:

'currency' => [
    'validators' => [
        [ 'type' => 'IsInt' ],
        [ 'type' => 'Between', 'options' => ['min' => 100, 'max' => 1000]],
    ],
]

Configuration-driven usage implies factories of some sort, and for ZF2, that has traditionally meant a plugin manager.

Getting rid of the plugin manager makes consumption harder:

We solved this partially in zend-feed, by introducing a component-specific plugin manager, and a default implementation that is standalone. This allows the most common, documented use case to "just work," while reducing a dependency. For those who want the more powerful features of zend-servicemanager, an implementation is present to allow doing that as well. I strongly urge that instead of removing the plugin manager entirely, we go this route instead, as it's the most common, most consumed use case.

One key point I want to make is that with the switch to component-based repos instead of a monolithic repo, one big goal is to try and increase standalone usage of components. We cannot assume users are using the full-stack, nor that they are familiar with ZF and all of its components. The simpler we can make the standalone consumption story, the more likely we'll get such adoption.

My point is that while it may look and feel more semantically correct to move the validators into different components, or to add a layer on top in order to achieve a proper separation of concerns, such architectural changes are detrimental to actual consumers and potential users of the component. We should be looking at how the components are currently used, how people might discover and use them, and write code that facilitates those paths as succinctly and easily as possible. Pragmatic architecture, please.

akrabat commented 9 years ago

For the record, I mostly care about getting the PluginManager instance out of the InputFilter instance. The factory should use the PluginManager to build all the things and give me back an InputFilter that's ready to go.

Also, I want the fully features PluginManager, so I'm fine with a ServiceManager dependency as it's really common to have custom validators.

Maks3w commented 9 years ago

@weierophinney Don't we have the same discovery problem since we split ZF2 in many components and every component is part of the organization ?

manuakasam commented 9 years ago

Partially @Maks3w, right now we have the "well known" components that we basically have since ZF1.

With more SoC we'd be ending up having zend-validator (only validator) zend-inputfilter (only if) zend-filter (only filter) zend-validation-bundle (ships above 3 and PluginManager connections?)

This requires you to KNOW about zend-validation-bundle.

While I appreciate the SoC as such and perfectly understand the ease it gives to the developers, it does make it more difficult for users. With this being said though: I also feel that this is somewhat of a documentation concern. When voicing it out loud enough, I can easily see this change happening, thus I am still :+1: to it.

Maks3w notifications@github.com schrieb am Mo., 20. Juli 2015 um 23:53 Uhr:

@weierophinney https://github.com/weierophinney Don't we have the same discovery problem since we split ZF2 in many components and every component is part of the organization ?

— Reply to this email directly or view it on GitHub https://github.com/zendframework/zend-validator/issues/1#issuecomment-123057179 .

Maks3w commented 9 years ago

I think in the following packages structure.

"zend-db", // suggest zend-db-validator
"zend-db-validator", // require zend-db and zend-validator. Suggest void
"zend-validator" // suggest zend-db-validator

Only the first two packages are directly required by the application "zend-db", "zend-db-validator"

Another alternative is keep empty validator classes in Zend\Validator extending from the external ones:

Zend\Validator\Db\Foo extends Zend\Db\Validator\Foo

weierophinney commented 9 years ago

@manuakasam —

I also feel that this is somewhat of a documentation concern.

The problem is: nobody reads the documentation, particularly not first. They look for a validation package, see if the README gives them enough to start with, install, and go. Having a "consumer" package that sits on top means they will not find it, or will feel it's "too much overhead", and they'll skip it.

In the real world, what I see often is that the more dependencies a package has, the fewer installs it will get. People judge the worth of a package in inverse proportion to the number of direct dependencies it has. We should be aiming to keep the component self-contained, not spreading it into more and more packages.

In particular, this proposal by @Maks3w :

"zend-db", // suggest zend-db-validator
"zend-db-validator", // require zend-db and zend-validator. Suggest void
"zend-validator" // suggest zend-db-validator

while it is technically beautiful, means installing not one, not two, but THREE packages in order to get DB-based validators.

Consider this, too: user encounters a bug in their zend-db-based validator. How do they determine where to fix it?

Believe me, I love the architecture. But I see it as presenting large usage hurdles to consumers, a hindrance to maintenance, and a hindrance to adoption. Again, pragmatic, user-focused architecture, please. I think the ideas we're seeing evolve on the proposal for the ResultInterface from @Maks3w are getting very user-focused at this point; we need that same focus at the package level.

Maks3w commented 9 years ago

What about to put specific integration logic in module?

We could create zend-validator-module and put their the plugin manager and require zend-*-validator packages

bakura10 commented 9 years ago

My only concern with the ideas of module is that we will need to tag them along the validators component as user will basically insert validator-module in their composer instead of validator. So whenever we upgrade validator to a major version the module will also need to be updated even if there is no change code in the module itself.

Envoyé de mon iPhone

Le 21 juil. 2015 à 08:37, Maks3w notifications@github.com a écrit :

What about to put specific integration logic in module?

We could create zend-validator-module and put their the plugin manager and require zend-*-validator packages

— Reply to this email directly or view it on GitHub.

akrabat commented 9 years ago

As someone who uses Zend\Validator outside of ZF2, it wouldn't occur to me to look for a ZF2 Module in order to make custom validators work with an array-based configuration.

The point about component dependencies is a very important one. I've seen a lot of devs refuse to use ZF2 components because of the number of dependencies in the composer.json.

I'm also interested to know if anyone uses Validator or Filter and doesn't also use InputFilter. There's a reasonable argument for a single component that combines the input filter chain, the validators and the filters.

Maks3w commented 9 years ago

The point about component dependencies is a very important one. I've seen a lot of devs refuse to use ZF2 components because of the number of dependencies in the composer.json.

I think the only way of break the suggest storm is to split subcomponents in their own packages.

akrabat commented 9 years ago

I'm not sure anyone ever looks at the suggest output - it's just noise.

Maks3w commented 9 years ago

suggest/require storm by the case is the same problem.

Too much dependencies than I need (require) or too few dependencies (suggest)

gianarb commented 9 years ago

I think the only way of break the suggest storm is to split subcomponents in their own packages.

This choice has already been made few time ago for zend components and now we have a very speedy release flow. In my opinion is a plus for the framework to split components from its integrations because it helps me to choice zend\validator how a good starting point for my validator library without manage integration with other components.. We can move all "blob" and all complexity of choices made to build a good mvc application flow in one or more modules..

I'm not sure anyone ever looks at the suggest output - it's just noise.

@akrabat I agree with you and it's for this reason that I would prefer download all dependencies one shot even if it is used by only one feature and don't follow dependencies when my app doesn't run

The dependencies are too many? This is another problem that require another solution

bakura10 commented 9 years ago

Interestingly, I'm having the same issue of being tied to a ZF config in the service manager refactor: https://github.com/zendframework/zend-servicemanager/pull/6/files#diff-3b686f846ce13d01f4766d488d019402L32

RalfEggert commented 8 years ago

What is the status of stateless validators?

bakura10 commented 8 years ago

This is way to big to handle for myself alone, a lot of cleaning, refactoring, and update tests is needed. If someone want to take from my work, the PR is still available here: https://github.com/zendframework/zf2/pull/5067/files

michalbundyra commented 4 years ago

This repository has been closed and moved to laminas/laminas-validator; a new issue has been opened at https://github.com/laminas/laminas-validator/issues/48.