Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Validation messages #13208

Closed
JABirchall opened this issue Dec 12, 2017 · 10 comments
Closed

Validation messages #13208

JABirchall opened this issue Dec 12, 2017 · 10 comments
Assignees
@niden
Labels
breaks bc Functionality that breaks Backwards Compatibility enhancement Enhancement to the framework new feature request Planned Feature or New Feature Request

Comments

@JABirchall
Copy link

Im not sure as to the reason behind this decision, but I feel that the validation messages for the included validators should be within the validators them selfs not the base class.

https://github.com/phalcon/cphalcon/blob/master/phalcon/validation.zep#L334-L360

This is a very minor issue and more of my ODD kicking in, just feel its better design to have each validator define its default message to allow for easier extension and reliable removal of validators.
@virgofx
Copy link
Contributor

virgofx commented Dec 12, 2017

Agreed. PRs are welcome. This should be a pretty straightforward refactor.

@JABirchall
Copy link
Author

I would if I knew how to work with zephir :P

@Jurigag
Copy link
Contributor

Jurigag commented Dec 14, 2017

Just clone repo, install zephir extension into phpstorm and code it and do pr. Nothing fancy really.

@stale stale bot added the stale Stale issue - automatically closed label Apr 16, 2018
@JABirchall
Copy link
Author

I feel this issue should stay open. @sergeyklay

@sergeyklay sergeyklay reopened this Jun 26, 2018
@stale stale bot removed the stale Stale issue - automatically closed label Jun 26, 2018
@stale stale bot added the stale Stale issue - automatically closed label Sep 24, 2018
@stale stale bot closed this as completed Sep 25, 2018
@niden niden reopened this Sep 25, 2018
@stale stale bot removed the stale Stale issue - automatically closed label Sep 25, 2018
@phalcon phalcon deleted a comment from stale bot Dec 22, 2018
@phalcon phalcon deleted a comment from stale bot Dec 22, 2018
@niden niden self-assigned this Dec 23, 2018
@niden niden mentioned this issue Feb 23, 2019
Closed
@niden
Copy link
Member

niden commented Feb 23, 2019

Closing in favor of #13855. Will revisit if the community votes for it, or in later versions.

@niden niden closed this as completed Feb 23, 2019
@emiliodeg
Copy link
Contributor

@niden can you assign me this?

@niden niden reopened this Apr 27, 2019
@niden
Copy link
Member

niden commented Apr 27, 2019

@emiliodeg all yours

@niden niden added the enhancement Enhancement to the framework label May 16, 2019
@emiliodeg
Copy link
Contributor

I've been working on this and I want to share with you my design to receive opinions before doing all the work

I use an advice property to warning message, to avoid confusion with Phalcon\Messages\Message

Some validators like Phalcon\Validation\Validator\File starts to work like a container and split the inside validations in new validatos like Phalcon\Validation\Validator\File\Size, Phalcon\Validation\Validator\File\MimeType, Phalcon\Validation\Validator\File\Resolution, etc

The main idea is to have a transparent transition. All current definitions will continue to work as they do now

@niden niden added the breaks bc Functionality that breaks Backwards Compatibility label May 16, 2019
@emiliodeg emiliodeg mentioned this issue Jun 20, 2019
Merged
5 tasks
niden added a commit to emiliodeg/cphalcon that referenced this issue Jun 20, 2019
@niden
[phalcon#13208] - Interface corrections/docblocks
50792a2
niden added a commit to emiliodeg/cphalcon that referenced this issue Jun 20, 2019
@niden
Revert "[phalcon#13208] - Interface corrections/docblocks"
c66b03f 
This reverts commit 50792a2.
niden added a commit to emiliodeg/cphalcon that referenced this issue Jun 20, 2019
@niden
[phalcon#13208] - Interface corrections
57bea9d
niden added a commit to emiliodeg/cphalcon that referenced this issue Jun 20, 2019
@niden
Revert "[phalcon#13208] - Interface corrections"
ecde2b5 
This reverts commit 57bea9d.
niden added a commit to emiliodeg/cphalcon that referenced this issue Jun 20, 2019
@niden
[phalcon#13208] - Trying interface corrections
975d1f7
niden added a commit to emiliodeg/cphalcon that referenced this issue Jun 20, 2019
@niden
[phalcon#13208] - Trying different scope
da8cd27
niden added a commit to emiliodeg/cphalcon that referenced this issue Jun 21, 2019
@niden
[phalcon#13208] - Trying to fix the observed var
9bf010b
niden added a commit to emiliodeg/cphalcon that referenced this issue Jun 21, 2019
@niden
[phalcon#13208] - Corrected references
cfc59f4
niden added a commit to emiliodeg/cphalcon that referenced this issue Jun 21, 2019
@niden
[phalcon#13208] - Corrected test
51ff213
niden added a commit to emiliodeg/cphalcon that referenced this issue Jun 21, 2019
@niden
[phalcon#13208] - More test corrections
d7bab7e
niden added a commit to emiliodeg/cphalcon that referenced this issue Jun 21, 2019
@niden
[phalcon#13208] - Corrected more tests
162fc84
niden added a commit that referenced this issue Jun 21, 2019
@niden
Merge branch 'T13208-validation-messages' of https://github.com/emili…
0d7fb93 
…odeg/cphalcon into emiliodeg-T13208-validation-messages

* 'T13208-validation-messages' of https://github.com/emiliodeg/cphalcon: (27 commits)
  [#13208] - Corrected more tests
  [#13208] - More test corrections
  [#13208] - Corrected test
  [#13208] - Corrected references
  [#13208] - Trying to fix the observed var
  [#13208] - Trying different scope
  [#13208] - Trying interface corrections
  Revert "[#13208] - Interface corrections"
  [#13208] - Interface corrections
  Revert "[#13208] - Interface corrections/docblocks"
  [#13208] - Interface corrections/docblocks
  Updated docblock
  Updated validators
  Fixed array fields
  Fixed some tests
  Changed advice to template
  Changed advice for template
  More string length tests
  String length tests
  Added a message factory
  ...
niden added a commit that referenced this issue Jun 21, 2019
@niden
Merge branch 'emiliodeg-T13208-validation-messages' into 4.0.x
b5e0185 
* emiliodeg-T13208-validation-messages: (27 commits)
  [#13208] - Corrected more tests
  [#13208] - More test corrections
  [#13208] - Corrected test
  [#13208] - Corrected references
  [#13208] - Trying to fix the observed var
  [#13208] - Trying different scope
  [#13208] - Trying interface corrections
  Revert "[#13208] - Interface corrections"
  [#13208] - Interface corrections
  Revert "[#13208] - Interface corrections/docblocks"
  [#13208] - Interface corrections/docblocks
  Updated docblock
  Updated validators
  Fixed array fields
  Fixed some tests
  Changed advice to template
  Changed advice for template
  More string length tests
  String length tests
  Added a message factory
  ...
@niden
Copy link
Member

niden commented Jun 21, 2019

Resolved in #14193

@niden niden closed this as completed Jun 21, 2019
@JABirchall
Copy link
Author

Thank you guys :)

@niden niden added the 4.0 label Jun 21, 2019
@niden niden added the new feature request Planned Feature or New Feature Request label Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@niden niden

Labels
breaks bc Functionality that breaks Backwards Compatibility enhancement Enhancement to the framework new feature request Planned Feature or New Feature Request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@virgofx @niden @emiliodeg @sergeyklay @JABirchall @Jurigag