Skip to content
This repository has been archived by the owner on Nov 23, 2017. It is now read-only.

Optimization and fixes for validation #63

Merged
merged 5 commits into from Mar 26, 2015
Merged

Optimization and fixes for validation #63

merged 5 commits into from Mar 26, 2015

Conversation

shadowhand
Copy link
Member

Several improvements and optimizations for component validation. Unit tests included for changes.

Using `[+-.]` becomes a character range that includes `+,-.` when the expected behavior
is character literals.
Existing code was allowing nearly anything to be used as username and password.
Applied a `UserComponentValidatorTrait` to User and Pass classes to conform to RFC spec.
@@ -28,8 +28,7 @@ class Port extends AbstractComponent implements Component
*/
protected function validate($data)
{
$data = filter_var($data, FILTER_VALIDATE_INT, ['options' => ['min_range' => 1]]);
if (! $data) {
if (! ctype_digit($data) || $data < 1) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm honestly not sure if this is better. It may be more brittle because ctype_* methods trigger warnings when the value is not a string. While the added type casting ensures it won't error, that might still be a good reason to revert this.

Copy link
Member

Choose a reason for hiding this comment

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

Yep I'm on the fence with this one too :)

nyamsprod added a commit that referenced this pull request Mar 26, 2015
@nyamsprod nyamsprod merged commit 52967ef into thephpleague:master Mar 26, 2015
@nyamsprod
Copy link
Member

Great additions. Thanks for the help making the package more RFC compliant. I'm gonna merge everything an just revert the ctype check on the Port component.

@nyamsprod
Copy link
Member

I've added more methods on URL and Path but overall I think the v4 is shaping good 👍 I've made some optimizations in regard to the use of punycode. We just need to test the package and update the documentation.
The only thing that is worrying me is the fact tha v4 is completely different from v3 in API and in spirit. So I'm wondering if we should or not move to another sub-namespace just in case.
I'm no saying that it is no possible but migrating a code from v3 to v4 will surely be difficult.
What do you think ?

@shadowhand shadowhand deleted the feature/validate-optimization branch March 27, 2015 04:44
@shadowhand
Copy link
Member Author

As toArray mimics the results of parse_url, when present the port must be a int.

Makes sense. I didn't realize that parse_url generated an int for port.

@shadowhand
Copy link
Member Author

The only thing that is worrying me is the fact tha v4 is completely different from v3 in API and in spirit.

That's the beauty of semver, you don't have to worry about breaking the API, because major version bumps are expected to break the API. If people don't want to upgrade, they can choose not to.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants