Database: removed refetching all columns on ActiveRow::toArray() (BC break!)

[hrach]

Removed some magic ported from NotORM.
Some discussion: http://forum.nette.org/cs/13703-mozna-chyba-v-cachovani-sloupcu-u-dotazu-v-nette-database
Correct workaroudn is to use ->select('*') to get all columns. Othrwise in current state is impossible to iterate only over fetched columns.

[enumag]

👍

[dg]

I am not sure this is good. Consider the unexpected behaviour of toArray(). Solution is update cache to fetch all columns next time.

[enumag]

@dg For me it's the current behaviour that is unexpected.

[hrach]

@dg well, yes I considered it a lot. If I call $row->toArray() I expect to get array of feteched column in row. Also the fact some queries need all columns is nonsence. They don't need it, they just don't know what they need - it's quite opposite what this feature should bring.

The toArray is pattern and refetching for all columns (in background of this method) is magic which I want to get rid of.

[hrach]

@dg After another day of thinking I am sure this is right. Changing strucutre can't fetch another data, it's just wrong. Only use case which came on my mind is geting data for session and then transforming to array... and that's exactly the case, when you should explicitly use select('*'), because you know! which columns you will need. Although I have idea for some better cache key composition based on routing, even in that case it would destroy caching mechanism, since this query could happened on more "urls".

[dg]

@enumag: Unexpected? Now it returns table row as array. Always the same array. With this commit, it returns some fields, nobody knows which ones.

[dg]

@hrach: Caching is transparent mechanism for accelerating of application. Function toArray() must return the same result whether caching is enabled or not. Otherwise it's behavior is unexpected.

[enumag]

@dg: Yes, unexpected. Have you seen this? When all columns are needed I prefer to specify it with select('*').

[dg]

@enumag this is bug, but this commit is not solution.

[hrach]

Main reason:

• toArray is quite standardized alias for iterator_to_array. These language contructs (imo) must behave the same.
• I strongly recommend to notice that toArray() "is method" for \Traversable. Not Selection, not SuperSmartSqlQuery. => this method must reflect \Traversable state, neither a table state nor database state.
• method name says "to array", not "something similar as array".

---

Comment to @dg arguments:

Now it returns table row as array.

No, wrong. $db->table('book')->select('id')->fetch()->toArray() return array('id' => ...). It does not represent table row, it just represents (correctly) \Traversable as a array.

Always the same array. With this commit, it returns some fields, nobody knows which ones.

Not exactly true. After this commits it returns just that what IRow contains. Not satisfied with this? Let Selection to fill Row with all columns.

Caching is transparent mechanism for accelerating of application. Function toArray() must return the same result whether caching is enabled or not. Otherwise it's behavior is unexpected.

Of course, yes, but that's all about understanding the meaning of ActiveRow and feature, which provides. The feature (to refetch needed columns) provides ActiveRow, not array.

---

Some problems connected with this magic:

$a = $db->table('author')->fetch();
unset($a->author);
$a->toArray()

• fuck - shoudl array contains author key? well, yes, we may detected unseted columns and then unset them from "all columns". (Don't forget about need to unset from unseted column every time you set some column... just imagin what's ActiveRow getting to be "a class").

$a = $db->table('author')->fetch();
$a->fullname = $a->firstname . $a->lastname;
$a->toArray();

• fuck possibility 1 - fullname is not present
• fuck possibility 2 - fullname is present, but object could be in inconsitent state, first name could change in db.

etc. etc. etc.

[dg]

@hrach please, do not fuck.

I'll try explain why this commit is totally fucking wrong. Delete cache in CD-collection example and sign in. The identity will contain 'username' field. Then sign out and sign in again. The 'username' field disappears. Oh fuck!

Yes, it can be fixed by adding select('*') on line https://github.com/nette/examples/blob/master/CD-collection/app/model/Authenticator.php#L33, but select('*') is default.

[enumag]

@dg: Username is used for authentication which means it will be accessed and therefore will be in the array. What you said is correct for firstname and lastname.

[dg]

@enumag try it

[enumag]

@dg srry it's used in where, you are correct

[vojtech-dobes]

What about inventing another method, something like ->getCompleteRow()? Because it's true that from @hrach 's point of view (as I understand it), it's based on "unknown", that ->toArray() works correctly in the example now.

[enumag]

However in my opinion the toArray method should not be used in this case. It'd be better to create the array manually with only needed data, not everything that might be in the table. You may also need to add some data from other tables.

@vojtech-dobes there is no need in my opinion, we have select('*') for that.

[dg]

ad unset(): I do not really like this ActiveRecord stuff. It would be better if properties were read-only and ActiveRow::$modified did not exist.

[hrach]

Please, provide someone real usecase which

• need use of toArray()
• query is costructed without knowledge, you will need all columns.

I do not know any of this usage.

---

Your provided example is bad! You just do it wrong. You know you need all columns, you shall use explicit select(). If you don't use select, you are sentected to this problem.

[dg]

Default option is "all columns".

[hrach]

ad unset(): I do not really like this ActiveRecord stuff. It would be better if properties were read-only and ActiveRow::$modified did not exist.

Well, I agree with you. I would removed it. But it would lead only to overusing toArray(). Not sure what to do with it. Maybe would be correct to do not allow new columns and do not allow unset them.

Default option is "all columns".

What about new API, which would enable this magic. Such as $selection->autoSelect(). I'm heding to the state, when ActiveRow would be just data envelope - this do not it easier.

[dg]

Autoselect is default too ;-)

I think current behavior is correct, the only problem is with cache. Cache should remember that we need all columns and avoid sending two database queries next time.

The fact that toArray() disables some acceleration mechanisms should be mentioned in documentation. „If you'd like to use toArray(), consider using select(...) method, otherwise all columns will be transfered.“

[hrach]

There is still problem with unset.

[dg]

Unset & modify is common problem, not just toArray problem.

    $row = $db->table('albums')->fetch();
    $row->title = 'x'; // or unset($row->title)
    echo $row->artist; // may reload data, value of $row->title is unknown

[hrach]

Yes, but toArray is the most common situation, when it happens.

What about allowing toArray only when there is a explicit select?