Define API params in order to be able to filter correctly #413
Conversation
| $all_params = $this->server->getAllParams([ | ||
| 'w' => '100', | ||
| 'p' => 'small', | ||
| 'invalid' => '1', |
There was a problem hiding this comment.
@ADmad I realize I added some fixes that are out of scope of the issue, but this is the test fixing the cache path issue
I believe it shouldn't accept invalid parameters
ADmad
left a comment
ADmad
left a comment
There was a problem hiding this comment.
no BC break
Adding a new interface method is very much a BC break 🙂
But I think it's worth it in this case, given the improvement it provides.
src/Api/Api.php
Outdated
| $params = []; | ||
|
|
||
| array_walk($this->manipulators, function (ManipulatorInterface $manipulator) use (&$params) { | ||
| foreach ($manipulator->getApiParams() as $param) { |
There was a problem hiding this comment.
Why loop through each instead of using array merge?
src/Api/Api.php
Outdated
| foreach ($manipulator->getApiParams() as $param) { | ||
| $this->apiParams[] = $param; | ||
| } |
There was a problem hiding this comment.
| foreach ($manipulator->getApiParams() as $param) { | |
| $this->apiParams[] = $param; | |
| } | |
| $this->apiParams = array_merge($this->apiParams, $manipulator->getApiParams()); |
There was a problem hiding this comment.
I was over-complicating this, thanks for pointing it out!
src/Manipulators/BaseManipulator.php
Outdated
| * | ||
| * @return list<string> | ||
| */ | ||
| abstract public function getApiParams(): array; |
There was a problem hiding this comment.
We don't need this abstract method because it is defined already in ManipulatorInterface
There was a problem hiding this comment.
I agree, I was following what was done for the run method, I removed both from BaseManipulator
3 participants
Uh oh!
There was an error while loading. Please reload this page.