Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

activerecord as array #819

Closed
wants to merge 19 commits into
from

Conversation

5 participants
Owner

cebe commented Jun 12, 2012

implementation of asArray as discussed in #531.
Allows returning arrays as result of all find methods and relations with getRelated().

Implementation introduces a scope-like method named asArray which can be used as follows:

<?php
    $post=Post::model()->asArray()->findByPk(1);
    $posts=Post::model()->asArray()->findAll();
    // This also works for relations (asuming <code>author</code> and <code>categories</code> are relations):
    $author=$post->asArray()->author;
    $categories=$post->asArray()->categories;

It will work for all find Methods, so I touched CActiveRecord::query(), CActiveRecord::findBySql() and CActiveRecord::findAllBySql().
Also fixed scope resetting ($this->_c=null) in findBySql which did not work properly.

adjustments to getRelated() diff

getRelated() will return array result, when scope asArray is true by using asArray implementation of activeFinder.
The $this->_related cache is not touched in case of asArray.

adjustments to query() diff

query will use the result of $command->queryAll()/$command->queryRow() to provide asArray functionallity. It will consider $criteria->index just like populateRecords() does.
If criteria has a with option set, it will use CActiveFinders asArray implementation.

asArray in CActiveFinder diff

CActiveFinder will use a new class CActiveRecordArray (see code in diff) which provides the same interface as CActiveRecord does for adding related records, getting primary key and getting attributes, so the changes to code of CActiveFinder can be reduced to a minimum. This class is necessary because CJoinElement::populateRecord() works on the objects by reference to attach related records. This is not possible with arrays, so I had to take the overhead of this class into to make it possible without having to change the whole implementation. As shown in a comment below this is not as good as pure array but much better than using CActiveRecord itself.
All places that create records now have to check if asArray is true and create CActiveRecordArray instead of CActiveRecord and all places where result is returned will use CActiveRecordArray::toArray() to return array result.
So CActiveRecordArray is only used internally by CActiveFinder and never comes to the outside (I am checking this in all of the tests).

issue #531 added asArray property to CActiveFinder
added asArray property to activeFinder to support returning query
results as arrays instead of records.
To fully support this, the API of CActiveRecord needs to be adjusted to
make use of this feature, also some places marked with @todo are not implemented yet.

This commit is ment to be a request for comments.

Using the ArrayAccess of CModel here helps to keep code readable. Otherwise there needs to be an extra if statement.

Using the ArrayAccess of CModel here helps to keep code readable. Otherwise there needs to be an extra if statement.

Owner

cebe commented on 7f7f511 Jun 11, 2012

except the @todo annotations this is all we need to do with ActiveFinder to enable array support.

cebe added some commits Jun 12, 2012

removed todo annotations
code already did what it should

related to #531
added missing implementation of getPrimaryKey
needed a getPrimaryKey() for array of attributes.

related to #531
issue #531: implemented asArray for CActiveRecord
Allows returning arrays as result of find methods.
details discussed in issue #531

@qiangxue qiangxue and 1 other commented on an outdated diff Jun 12, 2012

framework/db/ar/CActiveFinder.php
@@ -881,21 +908,48 @@ private function populateRecord($query,$row)
continue;
$childRecord=$child->populateRecord($query,$row);
@qiangxue

qiangxue Jun 12, 2012

Owner

When $childRecord is an array, changes to it will not be kept. This is the main difficulty of implementing this feature.

@cebe

cebe Jun 15, 2012

Owner

Yep, just recognized that... Should have written tests before implementing it :)

cebe added some commits Jun 15, 2012

added CActiveRecord::getAsArray()
added CActiveRecord::getAsArray() to be able to get the current state of
asArray for example in a behavior beforeFind().
added test class to test asArray feature
test covers all find methods of CActiveRecord with and without the with
option. So find with CActiveFinder + CActiveRecord is covered nearly
100%.
Currently STAT relations and Composite PK are not explicitly covered.

These tests compare results generated by normal call of CActiveRecord
find*()-methods with the result of find*()-methods in combination
with asArray()
It is not checked if the result is the correct one expected related
to the query criteria.
Owner

cebe commented Jun 15, 2012

Just added a test class in fc8d737 which covers nearly every detail of asArray feature. Currently all tests related to querys with with are failing. Will further work on that.

cebe added some commits Jun 18, 2012

reverted most of CActiveFinder::asArray
replaced with a better solution which might have a bit more memory
overhead but will integrate more easily and does not blow code
finished implementation of asArray in CActiveFinder
current approach uses a simplified class for storing ar's
data which is not as good as pure array but much better as direct AR
Owner

cebe commented Jun 18, 2012

Changed implementation to use a simplified AR class for storing the data. This one will be used when fetching data via CActiveFinder. It is not as good as arrays but much better that pure AR.
Here is a little test that shows memory usage(not a good test case but shows a bit what we can expect about memory usage).

<?php

        Yii::import('system.db.ar.CActiveFinder', true);
        Post::model()->getMetaData();
        User::model()->getMetaData();
        $attributes = array(
            'id'=>5,
            'title'=>'test title of a post',
            'create_time'=>time(),
            'author_id'=>25,
            'content'=>'Lorem ipsum dolor sit amet'
        );
        $attributes2 = array(
            'id'=>5,
            'name'=>'the author',
            'email'=>'mail@cebe.cc',
        );

        $old = memory_get_usage();
        $record0=$attributes;
        $mem = memory_get_usage();
        echo "\nmemory usage array only: \n".(abs($mem - $old)/1024).' kb'."\n";

        $old = memory_get_usage();
        $record1=new CActiveRecordArray($attributes,Post::model());
        $mem = memory_get_usage();
        echo "\nmemory usage array: \n".(abs($mem - $old)/1024).' kb'."\n";

        $old = memory_get_usage();
        $record2=Post::model()->populateRecord($attributes,false);
        $mem = memory_get_usage();
        echo "\nmemory usage AR: \n".(abs($mem - $old)/1024).' kb'."\n";

        $old = memory_get_usage();
        $record3=new CActiveRecordArray($attributes,Post::model());
        $record3->addRelatedRecord('author', new CActiveRecordArray($attributes2,User::model()),false);
        $mem = memory_get_usage();
        echo "\nmemory usage array with relation: \n".(abs($mem - $old)/1024).' kb'."\n";

        $old = memory_get_usage();
        $record4=Post::model()->populateRecord($attributes,false);
        $record4->addRelatedRecord('author', User::model()->populateRecord($attributes2,false),false);
        $mem = memory_get_usage();
        echo "\nmemory usage AR with relation: \n".(abs($mem - $old)/1024).' kb'."\n";
memory usage array only: 
0.046875 kb

memory usage array: 
0.8671875 kb

memory usage AR: 
19.46875 kb

memory usage array with relation: 
2.0390625 kb

memory usage AR with relation: 
22.359375 kb
Owner

cebe commented Jun 18, 2012

Btw: all test are passing will review and test the whole thing in some of my applications over the week, will give feedback on friday. I'm curious about your feedback, @qiangxue @samdark @mdomba !

Contributor

speedlog commented Jun 20, 2012

Nice work! :) Memory saving is impressive!

cebe added some commits Jun 22, 2012

Owner

cebe commented Jun 22, 2012

did some improvements and added tests for through and stat relation.
Also updated the pull request description, to make it easier for you to review my changes.

@cebe cebe commented on the diff Jun 29, 2012

framework/db/ar/CActiveRecord.php
+ * $posts=Post::model()->asArray()->findAll();
+ * </pre>
+ * This also works for relations (asuming <code>author</code> and <code>categories</code> are relations):
+ * <pre>
+ * $author=$post->asArray()->author;
+ * $categories=$post->asArray()->categories;
+ * </pre>
+ * The first examples will return an array of attributes each.
+ * The second examples will return a list of attribute arrays each.
+ * When {@link CDbCriteria::with} is set on the query criteria, the resulting array will contain these
+ * relations with the relation name as array key.
+ * @param boolean $array whether find result should be array of attributes instead of active record objects
+ * @return CActiveRecord the active record instance itself.
+ * @since 1.1.11
+ */
+ public function asArray($asArray=true)
@cebe

cebe Jun 29, 2012

Owner

btw: param is here to allow usage like this:

public function getTimezones($criteria, $asArray=false) {
    return Timezone::model()->asArray($asArray)->findAll($criteria);
}
Owner

cebe commented Jul 6, 2012

btw: will also write guide documentation about it when PR gets accepted.

Owner

cebe commented Jul 6, 2012

another btw: As these changes are very complex and maybe hard and time consuming to review I'd like to offer my help.
If it helps we can meet in a chatroom, irc or even skype voice chat and have a live discussion about it. So I am able to guide you through the changes.

Hi,

What is the status of this request? Is there any chance that this will be merged into the stable branch?

Owner

cebe commented Mar 26, 2013

Currently we did not agree on merging it and as it is a very comlex thing it costs much time to integrate and make sure it has no bad side effects.
From my side it will not make it into the core very soon, not sure if ever. I may merge master into this branch from time to time so you may checkout this branch in your project if you really need this feature.

Owner

samdark commented May 27, 2013

This one involved great effort to solve but I think we should not merge it.

@samdark samdark closed this May 27, 2013

Owner

cebe commented May 27, 2013

agreed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment