laminas-api-tools / api-tools-rest

Laminas Module providing structure for RESTful resources
https://api-tools.getlaminas.org/documentation
BSD 3-Clause "New" or "Revised" License
7 stars 16 forks source link

Wrong typing on fetchAll() #27

Closed ppaulis closed 2 years ago

ppaulis commented 2 years ago

Bug Report

Q A
Version(s) 1.6.1

Current behaviour

In: https://github.com/laminas-api-tools/api-tools-rest/blob/dde0d361e655a85bd07c150eb56976d6d6ac70ea/src/AbstractResourceListener.php#L183

the fetchAll() receives either an instance of Laminas\Stdlib\Parameters or an array.

The definition of the generated method stubs (and the parent method) is:

/**
     * Fetch all or a subset of resources
     *
     * @param  array $params
     * @return ApiProblem|mixed
     */
    public function fetchAll($params = [])
    {

Imho, in addition to problems with tools like PHPStan, this leads to all sorts of misunderstandings when trying to treat $params as an array. E.g. :

array_key_exists('my-key', $params)

generates :

Uncaught TypeError: array_key_exists(): Argument #2 ($array) must be of type array, Laminas\Stdlib\Parameters given

Expected behaviour

Wouldn't it be better to enforce a strict typing on Laminas\Stdlib\Parameters? And let the user decide if $params should be transformed into an array?

Thanks and greetings, Pascal

Ocramius commented 2 years ago

Would you be able to send a patch/proposal? Easier to review what you have in mind on a diff.

ppaulis commented 2 years ago

@Ocramius I can do that :+1: I'll get back to you asap.

ppaulis commented 2 years ago

@Ocramius I opened the PR #28. This is for discussion only because of the BC break it causes in the api-tools-admin repository. If you think this is worth pursuing, let me know how you'd like to handle it :-)

Ocramius commented 2 years ago

Handled in #28