Feature/validate input text #9
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
November 22, 2019 11:14
Create validateInput method that currently checks any inputs for null and throw an invalid argument exception. Check all methods that can error out by putting in null strings Create new tests for validating against null string
introwit
reviewed
Nov 22, 2019
| @@ -149,4 +159,17 @@ public function sanitizeLanguageCode(string $languageCode) | |||
| get the list of valid and supported language codes by running GoogleTranslate::languages()" | |||
| ); | |||
| } | |||
|
|
|||
| protected function validateInput($input): void | |||
There was a problem hiding this comment.
we should use the nullable type to typehint $input here ?string
There was a problem hiding this comment.
As you can pass an array through to it and validate it that way (rather than using 2 different methods) it'll break when using anything with batch
src/GoogleTranslate.php
Outdated
| protected function validateInput($input): void | ||
| { | ||
| if(is_array($input) && in_array(null, $input)) { | ||
| throw new \InvalidArgumentException('Input string cannot be null'); |
There was a problem hiding this comment.
can you import it at the top and remove \ on both lines
|
@RossUK88 although this is not something the package should be concerned of I merged it anyways because you had put in so much efforts and it was a non-breaking one. Tagged a new patch release. Please in the future, open an issue to discuss if something is out of the scope of this package 🙂 Thank you for the efforts though ❤️ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hey,
I came across a usecase where I was looping over some user content, and when they hadn't entered anything into a field I was getting an error (null string), while I had a try catch over this I wasn't get any feedback saying that it was invalid, so It was never caught.
While I do need to validate or not pass to the translate api myself I thought it'd be useful to have a Not Valid Arguement exception thrown for this.
This shouldn't break any code anyone has as it would be erroring out anyway.
Added in tests for the funationality too.
Extracted it to a method so if there are anymore things that should be validated agaisnt, they can be put in there with their exceptions.