nextras / dbal

Database Abstraction Layer – concise secure API to construct queries & fetch data
https://nextras.org/dbal
MIT License
79 stars 28 forks source link

Array access problem #55

Closed evolun closed 6 years ago

evolun commented 7 years ago

If I need an array access of rows fields I need a foreach loop for creating it by $row->toArray(), the fetchPairs('column', null) seems don't return the structure as shown in documentation but an array of row object instead:

$assoc = $result->fetchPairs('name', null); // [ // 'peter' => ['name' => 'peter', 'age' => 20], // 'john' => ['name' => 'john', 'age' => 13], // ]

It would be nice to have this chance also in fetchAll().

In my case I have troble with twig that return a NotSupportedException: Array access is suported only for indexed reading. Use property access.

hrach commented 7 years ago

Probably, I do not understand this sentence:

It would be nice to have this chance also in fetchAll().

Also, do you have some specific reason why you want to use array access instead of property access?

Of course, in documentation is a bug.

evolun commented 7 years ago

Apologies for my little clarity, also my not very good english affect :)

Anyway I was hasty, analyzing more deeply the problem I noticed the implementation of ArrayAccess interface in Row class, and I think is better handle offsetExists without exceptions but simply with boolean return, in case with a null return if you want to distinguish associative access from indexed.

My initial request is born from my attempt to integrate Nextras\Dbal into a slim framework application with twig template engine, which fire Nextras\Dbal\NotSupportedException when I try to access the data into a row from template, because twig handling his variable notation trying to access properties, methods or array items, so in detail a isset($row[$columnName]) throw exception.

It solved with little mod in Nextras\Dbal\Result\Row class:

public function offsetExists($offset)
{
    if (!is_int($offset)) {
        return false; // or eventually null
    }

    return $offset >= 0 && $offset < count((array) $this);
}
hrach commented 7 years ago

Could you elaborate why you need this behavior? Since Row does not support string key array access, it has no sence to call isset(). Such behavior may lead to hard to find bugs.

evolun commented 7 years ago

I don't need this behavior directly, twig need this (look here), inside the template engine is called isset().

If you are worried about bug handling you can throw a notice, otherwise this class remain incompatible with twig.

Personally I don't want leave twig, so I opted to dibi as db abstraction layer.

hrach commented 7 years ago

@evolun Thanks for the reply, finally I get it.

It's cause by nonsence behaviour, more described here in detail (gray block implementation).

@JanTvrdik what do you think about this? Compatibility with Twig seems to be a good argument to change the behavior. Or, we may introduce another row and enhanced api to change it, but it seems to be quite too complicated for such a small feature.

JanTvrdik commented 7 years ago

@hrach Do I understand correctly that the issue may be solved by either removing ArrayAccess or changing ArrayAccess behavior to match property access behavior? In that case vote for removing ArrayAccess implementation entirely – I never liked it in the first place.