-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Conversation
…ta set on aggregate filter
Hi @steverhoades The PR cannot be automatically merged. Can you merge the most recent master branch in your branch PR? |
@steverhoades Please rebase this PR onto the latest upstream master. The commit your PR is based on is over a year old. Also it'll make your life easier if you use a hotfix branch instead of getting your fork's master out of sync with upstream. ;) |
Woops. Thought it was a fresh fork, my bad. I'll take care of this shortly. |
Ok, 2nd try - did I do that right? |
It's mergeable. Thank you |
Perfect, sorry about that! A few comments regarding this commit, I think it would be nice to understand what the purpose of the populate() method is, as It's not abundantly clear to me why this method needs to exist. The same goes for validateInput($input). I went through the commit history but had a hard time extrapolating the details behind this. I think any dependency on $this->data should be removed from this class entirely, this could possibly make the getUnknown() and hasUnknown() methods irrelevant ... thoughts? |
- trailing whitespace - add parameter annotation - typehint on new parameter - spaces after operators, comments - remove extraneous lines in test
- trailing whitespace - add parameter annotation - typehint on new parameter - spaces after operators, comments - remove extraneous lines in test
This fixes issue #5270 where filters isValid returns no data present error when data is set on an aggregate inputfilter.