Skip to content

Conversation

@ErikBooij
Copy link
Contributor

The customer validator you could set on an Option wasn't triggered during option parsing, this required you to call $option->validate($option->value)) manually, which is cumbersome and counterintuitive.

Sidenote: I noticed PHPUnit wasn't included as a dependency in composer.json, any reason for that? It's hard to run the unit tests that way ;-)

@c9s
Copy link
Owner

c9s commented Feb 18, 2017

Thanks

The reason that phpunit was not added to the composer.json is that, I usually run the phpunit phar file directly

@ErikBooij
Copy link
Contributor Author

Ah, I see. Fair enough. I think for contributors it's easier to include it, but it's not that much of a hassle to install it in the project locally and just not commit it.

@c9s
Copy link
Owner

c9s commented Feb 18, 2017

you can add it to the dev dependency if it's more convenient

@c9s c9s merged commit fc60624 into c9s:master Feb 18, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 97.864% when pulling 65c83ae on ErikBooij:master into aa05d77 on c9s:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 97.864% when pulling 65c83ae on ErikBooij:master into aa05d77 on c9s:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 97.864% when pulling 6602b2d on ErikBooij:master into aa05d77 on c9s:master.

@c9s
Copy link
Owner

c9s commented Feb 22, 2017

@ErikBooij I just tagged a release for this fix https://github.com/c9s/GetOptionKit/releases/tag/2.5.1

}
}

if (!$this->validate($value)[0]) {
Copy link

Choose a reason for hiding this comment

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

This change breaks compatibility with PHP 5.3 which is still declared in composer.json. @c9s do you want to bump PHP requirements or fix this issue?

Copy link
Owner

Choose a reason for hiding this comment

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

@morozov shall we use an extra variable to improve the compatibility?

Copy link

Choose a reason for hiding this comment

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

Yes, I believe it will work.

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.

4 participants