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.

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

failValidationError can take an array of errors #4609

Merged
merged 6 commits into from
May 9, 2021

Conversation

dcaswel
Copy link
Contributor

@dcaswel dcaswel commented Apr 24, 2021

Description
Adjusted the ResponseTrait::failValidationError so it can take an array of validation errors

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately we cannot change the function signature like this because it will break any developer classes extending this. I do see how this functionality could be appealing though - how about adding a new method failValidationErrors with this functionality? We could even deprecate the old one so it could be removed down the road. See what the rest of the team thinks too...

Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from the point of @MGatner , the method's name itself implies using a single error so allowing an array of errors here seems unnatural. failValidationErrors is not a bad idea though. Maybe we can go that route?

…an array of errors instead of changing the signature for failValidationError.
@dcaswel
Copy link
Contributor Author

dcaswel commented Apr 24, 2021

@MGatner @paulbalandan Yeah, I was debating between the two approaches. I went the way I did because the body of both functions are exactly the same but I agree with both of your points so I pushed an update.

@dcaswel dcaswel requested a review from MGatner April 24, 2021 21:00
@MGatner
Copy link
Member

MGatner commented Apr 25, 2021

I would like both of your opinions on whether we should deprecate the string version.

@paulbalandan
Copy link
Member

I would like both of your opinions on whether we should deprecate the string version.

I don't have a strong opinion against deprecating it. Seems to me that having one authoritative version will be one less of a maintenance issue, so I'm good with deprecating the old one if you think so.

@dcaswel
Copy link
Contributor Author

dcaswel commented Apr 28, 2021

I could be swayed either way. I agree with @paulbalandan about having one less function means less to maintain. If we are going to do that, I would think we would want to change the signature for failValidationErrors() so it can accept an array or a string.

@paulbalandan
Copy link
Member

I could be swayed either way. I agree with @paulbalandan about having one less function means less to maintain. If we are going to do that, I would think we would want to change the signature for failValidationErrors() so it can accept an array or a string.

Hmm okay. I think that's our way to go. Please do what's necessary.

…ure of ResponseTrait::failValidationErrors so it can take a string as well.
@MGatner
Copy link
Member

MGatner commented May 8, 2021

@caswell-wc please also add this to user_guide_src/source/changelogs/v4.1.2.rst

@paulbalandan paulbalandan added the docs needed Pull requests needing documentation write-ups and/or revisions. label May 8, 2021
@paulbalandan paulbalandan removed the docs needed Pull requests needing documentation write-ups and/or revisions. label May 9, 2021
@MGatner MGatner merged commit 2c9c9b2 into codeigniter4:develop May 9, 2021
@MGatner
Copy link
Member

MGatner commented May 9, 2021

Thanks @caswell-wc !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants