dibi

dibi copied to clipboard

Version: current 4.1

Bug Description

When using Result::fetchAssoc in conjunction with Result::setRowFactory, fetchAssoc may throw exception Unknown column 'foo' in associative descriptor. even though the property is accessible when accessing $row->foo via magic getters.

Steps To Reproduce

        $result->setRowFactory(function($data){
            return new class($data) {
                use Strict;
                private $data;
                public function __construct($data)
                {
                    $this->data = $data;
                }
                public function __get(string $name)
                {
                    return $this->data[$name] ?? $this->data->{$name} ?? null;
                }
            };
        });
        array_map(function($v){return $v->foo;},$result->fetchAll()); // works just fine
        $result->fetchAssoc('foo'); // will throw

Unfortunatelly, this is quite a common pattern with many ORMs. 😥

The culprit

the problem is in Result class, where using property_exists with magic props will fail, quoted:

The property_exists() function cannot detect properties that are magically accessible using the __get magic method.

Possible Solution

I'm not yet sure what a backward compatible solution should look like.

This issue most probably plagues the Result::fetchPairs method as well, since property_exists is used there too.

I was wondering if I could come up with my own solution to the problem, however, as many times before, I hit the wall called "private" or "final".

First, I could post a PR where one could set the result class created by the Connection class instead of hard-wired Dibi\Result. Simple. Then one could provide his own result class to overcome this issue.

However, since both fetchPairs and fetchAssoc are final, it would either involve lots of code duplication or lots of code rewrite in the Dibi library itself.

I would like to make Dibi support entities that use magic getters, which is quite common use case. What is your snace on this matter, David? @dg

I think that fetchAssoc is not suitable for entities. Better way is to create new fetchEntities() function. (It does not necessarily have to be a method of the Result class descendant).

Consider this case. Really simple and useful.

I want to fetch users and then have them automatically sorted into groups by company and account type. Somewhere in the database layer there is this code to fetch users:

$allUsers = $connection->query('SLEECT * FROM `users` WHERE ... ')
    ->setRowFactory(function($row){ return new UserEntity($row); })
    ->fetchAssoc('company|accountType|id');

Then somewhere else in the application where I only work with entities, there might be code to access users or iterate over them $allUsers[$company][$type][$userId].

To support this, I have to make a 3-levels nested foreach cycle to convert the result to entities, and what is worse, the level of nesting changes depending on how the argument to fetchAssoc looks... 👎

$result = [];
foreach($allUsers as $companyId => $group){
    foreach($group as $type => $users){
        foreach($usersas $userId => $row){
            $result[$companyId][$type][$userId] = new UserEntity($row);
        }
    }
}
return $result;

Why would not this be a supported case for fetchAssoc?

I forgot to point out that the the suggested fetchEntities function would have to know the depth of nesting of the result, which it does not know.

And I consider parsing the argument to fetchAssoc a hack 🙂

The solution would be to modify fetchAssoc to

• not use $this->fetch() directly, but fetch row data via $this->getResultDriver()->fetch(true);
• do all tranformations
• apply $this->rowFactory & rowClass to the leaf, ie. the last item in structure

WIP 6b342624d71a471f4d4514d3e6683b681913addc

So bascally you modify the order to do the transformations first, then apply the row factory. Well at a point in time I found it strange that the row factory was invoked before fetchAssoc transformed the result, but it slipped of my mind. If this is not a BCB, then it seems more elegant to me. Since Result is tightly coupled to Connection, it should not be a BCB, at least not the the public interface. Good job.

In fact, it is BC break :-/ See #284

In my opinion, this cannot be solved without BC. And I don't really want to deal with it :-)

I wonder ... it's quite common to have an entity class put all the "row data" into one attribute and use magic accessors.

So you had the very same idea of soluton here #284, if i understand it correctly, which trned out to be a BCB.

---

Would making the result class configurable in Connection and removing the final modifiers from those functions in Result class be a BC break? that would allow us to modify the behaviour on the app side.

Furthermore, if there was an interface introduced that would be used along with the property_exists call, then that could solve the issue without a BC break, at the expense of adding a new interface to the library.

Something like the following.

// check columns
foreach ($assoc as $as) {
    // offsetExists ignores null in PHP 5.2.1, isset() surprisingly null accepts
    if (
        $as !== '[]' && $as !== '=' && $as !== '->' && $as !== '|' && !property_exists($row, $as) &&
        (!$row instanceof ExposesProperties || !$row->hasProperty($as))
    ) {
        throw new \InvalidArgumentException("Unknown column '$as' in associative descriptor.");
    }
}

interface ExposesProperties {
    public function hasProperty(string $name): bool;
}

The interface could then be implemented by the entity classes.

Yes, I'm afraid any change can be a BC break. New interface seems like a ways how to solve. I have to think about it more and unfortunately I don't have time for it now.

Related probem is with ->setRowClass(null). In that case, fetchAssoc() throws TypeError: property_exists(): Argument #1 ($object_or_class) must be of type object|string, array given