phurni / mortimer-poignant

Eloquent on steroids, implements RoR like ActiveRecord features as traits, including in-model validations, user stamping, declarative relations, cascaded operations on relations (save, delete).
MIT License
1 stars 1 forks source link

Cache results to not query on each call. #3

Closed RomainLanz closed 9 years ago

RomainLanz commented 9 years ago

Change done.

Concerning the bootExistingAttributesPersistence() method. As you can see bellow we win some queries.

Without condition: capture d ecran 2015-02-13 a 17 00 46

With condition: capture d ecran 2015-02-13 a 17 01 49

phurni commented 9 years ago

Let's merge this PR for UserStamping, please revert your changes for src/ExistingAttributesPersistence.php, there are only whitespace changes which is a no-op.

Concerning the log, we should first investigate where the second query is triggered, maybe this is a dev environment only situation.

RomainLanz commented 9 years ago

All changes made in src/ExistingAttributesPersistence.php are reverted.

RomainLanz commented 9 years ago

What do you think of changing the bootExistingAttributesPersistence() method with something like this:

public static function bootExistingAttributesPersistence()
{
    // Listen to the 'saving' event but with a low priority so that any validation is performed before purging the attributes
    $name = get_called_class();
    static::getEventDispatcher()->listen("eloquent.saving: {$name}", function($model)
    {
        // Keep only the attributes that exists in the table
        $model->setRawAttributes(
            array_only(
                $model->getAttributes(),
                \DB::connection()->getSchemaBuilder()->getColumnListing($model->getTable())
            )
        );
    }, -100);
}

With that, the query is executed only when you save the model and not at every load.

Or maybe better to create another Trait named StoreColumnsTrait that store all columns in a static property. And all other Trait will depend on this. With that you have only one request per model for all your Trait.

<?php namespace Mortimer\Poignant;

trait ExistingAttributesPersistence {

    use StoreColumnsTrait;

    /**
     * Boot the trait for a model.
     *
     * @return void
     */
    public static function bootExistingAttributesPersistence()
    {
        // Listen to the 'saving' event but with a low priority so that any validation is performed before purging the attributes
        $name = get_called_class();
        static::getEventDispatcher()->listen("eloquent.saving: {$name}", function($model)
        {
            // Keep only the attributes that exists in the table
            $model->setRawAttributes(
                array_only(
                    $model->getAttributes(),
                    self::getDatabaseColumns()
                )
            );
        }, -100);
    }
}
<?php namespace Mortimer\Poignant;

trait StoreColumnsTrait {

    /**
     * All columns for the related model.
     *
     * @var array
     */
    private static $databaseColumns = [];

    /**
     * Gets columns name of the related model.
     * 
     * @return array
     */
    public static function getDatabaseColumns()
    {
        if (!self::$databaseColumns) {
            self::$databaseColumns = \DB::connection()->getSchemaBuilder()->getColumnListing((new static)->getTable())
        }

        return self::$databaseColumns;
    }

}
phurni commented 9 years ago
  1. Yes, the tablecolumns should be memoized when they are used, not before. Your proposal doesn't memoize though. (Maybe in reference to 2.)
  2. I had the same "vision" when developing these traits, I think (prove me wrong please) this is not possible because a trait can only be use'd once in a class. So multiple traits using another one (StoreColumnsTrait) in the same class will fail at compile time. This limitation certainly has to do with method precedence in the trait system.
RomainLanz commented 9 years ago

You are true. Maybe better to add theses functions on the Core of Laravel.