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

Validators: added isListOfType() #123

Closed
wants to merge 6 commits into from
Closed

Conversation

@dstrop
Copy link
Contributor

dstrop commented Nov 5, 2016

I would like to resolve #119.

dstrop added 4 commits Nov 5, 2016
@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented Nov 6, 2016

You either need to rename the method to isIterableOfType($value, $type) or check whether the value is a list (by calling self::isList($list))

@TomasVotruba

This comment has been minimized.

Copy link

TomasVotruba commented Nov 6, 2016

I would expect this method to support objects as well:

Validators::isIterableOfType($items, Product::class);

For inspiration:

  • in PHPUnit, there is assertInternalType for string/int/bools.
  • in beberlei/assert is allIsInstanceOf
@dstrop

This comment has been minimized.

Copy link
Contributor Author

dstrop commented Nov 6, 2016

I thought about it but decided the object should implement Travarsable if you want to iterate through its properties. As is in new php7.1 function is_iterable.

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented Nov 6, 2016

@TomasVotruba I think that the example you've posted should work just fine with the current implementation.

@dg

This comment has been minimized.

Copy link
Member

dg commented Nov 7, 2016

What about everyIs?

@TomasVotruba

This comment has been minimized.

Copy link

TomasVotruba commented Nov 7, 2016

@JanTvrdik Ah, that is missing in tests, that suggest only scalar types are supported.

@t0his Could you please add test for simple object check? Sth like:

test(function () {
    Assert::true(Validators::isIterableOfType([new Product], Product::class));
    Assert::false(Validators::isIterableOfType([new Product, new stdClass], Product::class));
});
@dstrop

This comment has been minimized.

Copy link
Contributor Author

dstrop commented Nov 7, 2016

@TomasVotruba Sure i can add it, but shouldnt this be covered by is() tests?

@dg Ok i can rename it once i get home.

@dg

This comment has been minimized.

Copy link
Member

dg commented Nov 7, 2016

It was only a suggestion, hold on.

@TomasVotruba

This comment has been minimized.

Copy link

TomasVotruba commented Nov 7, 2016

@t0his Most part of the test is already covered by is(), but that's not the goal.
The test would be easier to understand, if both use cases - scalar and object - would be covered.

@dstrop

This comment has been minimized.

Copy link
Contributor Author

dstrop commented Nov 7, 2016

@TomasVotruba Ok would you like to add some other use case?

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented Nov 7, 2016

Maybe extract method isIterable + support is($value, 'iterable')?

@dstrop

This comment has been minimized.

Copy link
Contributor Author

dstrop commented Nov 7, 2016

But the method will soon be redundant with php7.1.

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented Nov 7, 2016

Yes, the method would use is_iterable and fallback to implementation in PHP in PHP < 7.1; probably best to make the method private to allow removal in future without breaking BC.

@TomasVotruba

This comment has been minimized.

Copy link

TomasVotruba commented Nov 7, 2016

@t0his Nope, I guess that's all I would expect this method to do. Thank you.

@dg dg closed this in 8b2fd88 Dec 19, 2016
*/
private static function isIterable($value)
{
if (function_exists('is_iterable')) {

This comment has been minimized.

Copy link
@dg

dg May 2, 2017

Member

Please use simply return is_array($value) || $value instanceof \Traversable;, function_exists is unnecessary complication.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.