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

Between Filter should have a ->count that is correct #77

Closed
creuzerm opened this issue Nov 8, 2017 · 8 comments
Closed

Between Filter should have a ->count that is correct #77

creuzerm opened this issue Nov 8, 2017 · 8 comments
Assignees
Milestone

Comments

@creuzerm
Copy link

creuzerm commented Nov 8, 2017

I think the between filter should output the correct count of dates as found by the filter. (Likely desired behavior for all filters, I haven't used any of the others yet)


$holidays = Yasumi\Yasumi::create('USA', 2017);
$holidays_Today = $holidays->between(new DateTime("11/01/2017"), new DateTime("12/31/2017"));
echo $holidays_Today->count(); 

11

I would expect this value to be 3 based on the holiday dates calculated.

@stelgenhof
Copy link
Member

Do you mean the between filter currently doesn't return the correct number of holidays?

@creuzerm
Copy link
Author

creuzerm commented Nov 9, 2017

Correct. The between filter doesn't return the correct number of holidays. In the example I gave above, it is returning 11 when I expect 3.

@stelgenhof
Copy link
Member

Oh, that's not good. Let me check that.

@stelgenhof
Copy link
Member

The between Filter is based on the FilterIterator class and returns an Iterator Object. The ->count() function doesn't get propagated to the an Iterator class (See also this bug report: https://bugs.php.net/bug.php?id=70113).

If you use the iterator_count function it will return the actual (filtered) number of holidays, like this:

echo iterator_count($holidays_Today): // 3

@stelgenhof
Copy link
Member

stelgenhof commented Nov 10, 2017

Following up on my previous comment: I've made a quick test implementing the Countable Interface and looks like it's working if you're using the count() function. I just need to create some unit tests and apply to the other filters as well.

@stelgenhof stelgenhof self-assigned this Nov 10, 2017
@stelgenhof stelgenhof added this to the v1.7.0 milestone Nov 10, 2017
@creuzerm
Copy link
Author

Thank you. Would adding an example of this to the filter cookbook page make sense? It would be helpful for people like me that are only fair coders and dont know what to expect from the iterator class.

@stelgenhof
Copy link
Member

I have implemented the Countable interface allowing you to use the ->count() method. For the moment I have only done it for the BetweenFilter (in the master branch). The other filters will be updated soon and also the documentation :)

Please have a look and let me know how it goes :)

@stelgenhof
Copy link
Member

All filters have now the Countable interface implemented. Available in the master branch and soon also in the upcoming v1.7.0 release.

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

No branches or pull requests

2 participants