[RFC] removing the validation part #94

Closed
willdurand opened this Issue Dec 16, 2011 · 18 comments

Comments

Projects
None yet
@willdurand
Member

willdurand commented Dec 16, 2011

Hi

I think we should remove the whole validation part. It's not the Propel's job IMO.
It's not really up to date and it causes some issues (See Propel 1.6 issues).

Propel2 has to focus on what it's designed for. The (Symfony2) Validator Component is built to perform validation, if someone wants to validate things, he could use this component.

Thoughts ?

William

@themouette

This comment has been minimized.

Show comment
Hide comment
@themouette

themouette Dec 16, 2011

Member

Hi,

Validation can be classifed in 2 parts :

  • business logic handeled by controlers, forms or whatever the dev want to
    use
  • data consistency

removing deprecated code is alright, but IMHO ORM has to be able to
validate data consistency (even through a behavior turned off by default)
and define basic validation in the schema.

Relying on a third party library such as Validator Component is a good
option to handle validation as we should not reinvent the wheel.

What do you think ?

Julien.

On Fri, Dec 16, 2011 at 8:28 AM, William DURAND <
reply@reply.github.com

wrote:

Hi

I think we should remove the whole validation part. It's not the Propel's
job IMO.
It's not really up to date and it causes some issues (See Propel 1.6
issues).

Propel2 has to focus on what it's designed for. The (Symfony2) Validator
Component is built to perform validation, if someone wants to validate
things, he could use this component.

Thoughts ?

William


Reply to this email directly or view it on GitHub:
#94

Member

themouette commented Dec 16, 2011

Hi,

Validation can be classifed in 2 parts :

  • business logic handeled by controlers, forms or whatever the dev want to
    use
  • data consistency

removing deprecated code is alright, but IMHO ORM has to be able to
validate data consistency (even through a behavior turned off by default)
and define basic validation in the schema.

Relying on a third party library such as Validator Component is a good
option to handle validation as we should not reinvent the wheel.

What do you think ?

Julien.

On Fri, Dec 16, 2011 at 8:28 AM, William DURAND <
reply@reply.github.com

wrote:

Hi

I think we should remove the whole validation part. It's not the Propel's
job IMO.
It's not really up to date and it causes some issues (See Propel 1.6
issues).

Propel2 has to focus on what it's designed for. The (Symfony2) Validator
Component is built to perform validation, if someone wants to validate
things, he could use this component.

Thoughts ?

William


Reply to this email directly or view it on GitHub:
#94

@ghost ghost assigned willdurand Dec 16, 2011

@willdurand

This comment has been minimized.

Show comment
Hide comment
@willdurand

willdurand Dec 16, 2011

Member

data consistency

Did you ever use the bundled validation in Propel 1.x ?
Doctrine2 doesn't handle validator neither in DBAL, nor in ORM.

If we focus on the ORM part, then we'll be able to provide a really powerful product. I think we should remove this validation without adding a new validation layer. I prefer to remove some features in order to add more useful ones like behaviors or something else.

Member

willdurand commented Dec 16, 2011

data consistency

Did you ever use the bundled validation in Propel 1.x ?
Doctrine2 doesn't handle validator neither in DBAL, nor in ORM.

If we focus on the ORM part, then we'll be able to provide a really powerful product. I think we should remove this validation without adding a new validation layer. I prefer to remove some features in order to add more useful ones like behaviors or something else.

@fzaninotto

This comment has been minimized.

Show comment
Hide comment
@fzaninotto

fzaninotto Dec 16, 2011

Member

I disagree. Propel can be used without a framework, and knowledge of the model can help generate simple validators. For the past two years, I have seen many people use Propel validators, and that proves that it's useful - although I agree it's flawed.

But Propel needs a lightweight validator generator IMHO.

Member

fzaninotto commented Dec 16, 2011

I disagree. Propel can be used without a framework, and knowledge of the model can help generate simple validators. For the past two years, I have seen many people use Propel validators, and that proves that it's useful - although I agree it's flawed.

But Propel needs a lightweight validator generator IMHO.

@willdurand

This comment has been minimized.

Show comment
Hide comment
@willdurand

willdurand Dec 16, 2011

Member

The Validator component is standalone. Why not using it ?

I definitely know Propel2 can be used without any frameworks. My mind is clear here.

To add a lightweight validator part is a bad idea, we won't reinvent the wheel. Also it seems weird to use the Validator component in Propel itself… People use Propel validators because there was anything else as a standalone lib. Now there is, so keep it simple, and don't reinvent a framework for Propel :)

Member

willdurand commented Dec 16, 2011

The Validator component is standalone. Why not using it ?

I definitely know Propel2 can be used without any frameworks. My mind is clear here.

To add a lightweight validator part is a bad idea, we won't reinvent the wheel. Also it seems weird to use the Validator component in Propel itself… People use Propel validators because there was anything else as a standalone lib. Now there is, so keep it simple, and don't reinvent a framework for Propel :)

@willdurand

This comment has been minimized.

Show comment
Hide comment
@willdurand

willdurand Dec 23, 2011

Member

I'm back on that issue and I think more and more that we have to remove the validation part.
There is no need to keep a plain old validation that is probably not up to date.

Once again, the Validator Component is designed for that part and works as a standalone lib. For those who use Propel without a Framework, they can rely on this component too. It will be widely better for us.

Member

willdurand commented Dec 23, 2011

I'm back on that issue and I think more and more that we have to remove the validation part.
There is no need to keep a plain old validation that is probably not up to date.

Once again, the Validator Component is designed for that part and works as a standalone lib. For those who use Propel without a Framework, they can rely on this component too. It will be widely better for us.

@hhamon

This comment has been minimized.

Show comment
Hide comment
@hhamon

hhamon Dec 23, 2011

Member

I'm +1 for this.

1- This is not the job of the ORM. The ORM just converts a relational model to an OOP model.
2- The database vendor can have validation features, why don't use them?
3- For me, it's the role of the developer to take care of the data he wants to persist to the storage engine.

Don't make Propel heavier... I'm pretty sure that people who already use the validation stuff in Propel 1.x won't migrate their old code to Propel 2.0. It would be a huge amount of code to write and there is probably no need for that. So, as Propel 2 is not yet stable we can make it lighter and even faster by removing unneeded tools.

We should try to keep Propel as thin and light as possible to ease support. Maybe these extra tools could be released as behaviors or third party tools like Doctrines does with migrations for example.

Member

hhamon commented Dec 23, 2011

I'm +1 for this.

1- This is not the job of the ORM. The ORM just converts a relational model to an OOP model.
2- The database vendor can have validation features, why don't use them?
3- For me, it's the role of the developer to take care of the data he wants to persist to the storage engine.

Don't make Propel heavier... I'm pretty sure that people who already use the validation stuff in Propel 1.x won't migrate their old code to Propel 2.0. It would be a huge amount of code to write and there is probably no need for that. So, as Propel 2 is not yet stable we can make it lighter and even faster by removing unneeded tools.

We should try to keep Propel as thin and light as possible to ease support. Maybe these extra tools could be released as behaviors or third party tools like Doctrines does with migrations for example.

@ClementGautier

This comment has been minimized.

Show comment
Hide comment
@ClementGautier

ClementGautier Dec 23, 2011

I agree with @willdurand : less code is less bugs, support duplicates validators doesn't make sense, its the job of the Validation component to do this.

I agree with @willdurand : less code is less bugs, support duplicates validators doesn't make sense, its the job of the Validation component to do this.

@themouette

This comment has been minimized.

Show comment
Hide comment
@themouette

themouette Dec 23, 2011

Member

All right with removing propel internal validators, but imho there should
be a way generate validation methods, configurable in the schema and
relying on a maintained third party library such as the Validator Component.

This could be a behavior, or in the core, but it has to be available
somehow.

Member

themouette commented Dec 23, 2011

All right with removing propel internal validators, but imho there should
be a way generate validation methods, configurable in the schema and
relying on a maintained third party library such as the Validator Component.

This could be a behavior, or in the core, but it has to be available
somehow.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Dec 23, 2011

I think it would be better to provide a hook allowing to use an existing validator instead of maintaining another one (which is not what Propel is about). Every PHP framework probably have a validator, and the Symfony2 Validator component is standalone (maybe other validators too but I don't know them).
Having something like the PrePersist and PreUpdate Doctrine events would achieve this.

AFAIK, such hooks don't exist in Propel 1.6 as the only hooks (I know of) are inside the model class and so cannot use external dependencies (except by making them statically available which is painful when the app is designed around the DI pattern)

stof commented Dec 23, 2011

I think it would be better to provide a hook allowing to use an existing validator instead of maintaining another one (which is not what Propel is about). Every PHP framework probably have a validator, and the Symfony2 Validator component is standalone (maybe other validators too but I don't know them).
Having something like the PrePersist and PreUpdate Doctrine events would achieve this.

AFAIK, such hooks don't exist in Propel 1.6 as the only hooks (I know of) are inside the model class and so cannot use external dependencies (except by making them statically available which is painful when the app is designed around the DI pattern)

@harikt

This comment has been minimized.

Show comment
Hide comment
@harikt

harikt Dec 23, 2011

Contributor

I agree with @stof probably a hook , so can use one validator or the other .

Contributor

harikt commented Dec 23, 2011

I agree with @stof probably a hook , so can use one validator or the other .

@guilhermeaiolfi

This comment has been minimized.

Show comment
Hide comment
@guilhermeaiolfi

guilhermeaiolfi Dec 23, 2011

I've never used validators in Propel 1.x. So, no problem for me.

I've never used validators in Propel 1.x. So, no problem for me.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Dec 23, 2011

+1 from me too for removing the validation. I never used it in 1.x either.

ghost commented Dec 23, 2011

+1 from me too for removing the validation. I never used it in 1.x either.

@cristianoc72

This comment has been minimized.

Show comment
Hide comment
@cristianoc72

cristianoc72 Dec 23, 2011

Member

I agree with @themouette . I always use Propel 1.x validators and i find wonderful to configure and generate validators via schema.xml file.

Member

cristianoc72 commented Dec 23, 2011

I agree with @themouette . I always use Propel 1.x validators and i find wonderful to configure and generate validators via schema.xml file.

@fzaninotto

This comment has been minimized.

Show comment
Hide comment
@fzaninotto

fzaninotto Dec 24, 2011

Member

Please, ask this question on the Propel Users mailing-list and you will see people who do use this feature.

I would agree with removing the current validation layer only if we provided a similar feature with another tool. Propel users must be able to define a validation format in their schema - not via annotations or classes.

Member

fzaninotto commented Dec 24, 2011

Please, ask this question on the Propel Users mailing-list and you will see people who do use this feature.

I would agree with removing the current validation layer only if we provided a similar feature with another tool. Propel users must be able to define a validation format in their schema - not via annotations or classes.

@nnarhinen

This comment has been minimized.

Show comment
Hide comment
@nnarhinen

nnarhinen Dec 24, 2011

Contributor

I would vote for removing the current implementation but would introduce core behaviors for validation.

For instance

<behavior name="unique_column_validator" />
<behavior name="autogenerate_validators" />
<behavior name="email_validator"><....column="email" /></behavior>

etc..

These behaviors could then use some 3rd party library, if the dependencies would be light. For the runtime I don't feel comfortable with adding more "hard-coded" dependencies.

Contributor

nnarhinen commented Dec 24, 2011

I would vote for removing the current implementation but would introduce core behaviors for validation.

For instance

<behavior name="unique_column_validator" />
<behavior name="autogenerate_validators" />
<behavior name="email_validator"><....column="email" /></behavior>

etc..

These behaviors could then use some 3rd party library, if the dependencies would be light. For the runtime I don't feel comfortable with adding more "hard-coded" dependencies.

@scp

This comment has been minimized.

Show comment
Hide comment
@scp

scp Dec 27, 2011

Agree with removal and I like @stof's idea of a hook for use with (e.g.) Symfony2 Validator. No point in duplicated efforts, and a lighter/thinner Propel 2.0 with more concentration on the ORM sounds preferable.

scp commented Dec 27, 2011

Agree with removal and I like @stof's idea of a hook for use with (e.g.) Symfony2 Validator. No point in duplicated efforts, and a lighter/thinner Propel 2.0 with more concentration on the ORM sounds preferable.

@cristianoc72

This comment has been minimized.

Show comment
Hide comment
@cristianoc72

cristianoc72 Mar 13, 2012

Member

Hi,
I've bootstrapped a validate-behavior, according to this issue. You can find it on my branch "validate-behavior" https://github.com/cristianoc72/Propel2/tree/validate-behavior. I also wrote a draft document in documentation/behavior dir. Any suggestion is welcome (about my awful English, too).

A couple of notes:

  1. this behavior is based on Symfony 2 Validator Component
  2. this branch is the result of merging @willdurand's remove-validate branch into master branch

ToDo:

  1. implementing your suggestions
  2. unit tests
  3. on the road tests
  4. improve documentation

Cristiano

Member

cristianoc72 commented Mar 13, 2012

Hi,
I've bootstrapped a validate-behavior, according to this issue. You can find it on my branch "validate-behavior" https://github.com/cristianoc72/Propel2/tree/validate-behavior. I also wrote a draft document in documentation/behavior dir. Any suggestion is welcome (about my awful English, too).

A couple of notes:

  1. this behavior is based on Symfony 2 Validator Component
  2. this branch is the result of merging @willdurand's remove-validate branch into master branch

ToDo:

  1. implementing your suggestions
  2. unit tests
  3. on the road tests
  4. improve documentation

Cristiano

@willdurand

This comment has been minimized.

Show comment
Hide comment
@willdurand

willdurand Mar 13, 2012

Member

Awesome!

Member

willdurand commented Mar 13, 2012

Awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment