model caching #417

shmax · 2014-07-01T02:59:05Z

Adds basic model caching functionality to active record. To enable caching for a particular model, set a static variable called "cache" to true in your model subclass, and configure a cache adapter. Set a static variable called "cache_expire" to a value in seconds; otherwise your Cache configuration expire period will be used.

koenpunt · 2014-07-01T08:04:19Z

Good, this one looks better. Can you add some tests for this as well?

cvanschalkwijk · 2014-07-01T16:53:07Z

lib/Model.php

+	protected function update_cache(){
+		$table = static::table();
+		if($table->cache_model){
+			Cache::set($this->cache_key(), $this, 0);


Expiration time should be an option and default to something other than never.

shmax · 2014-07-02T03:43:00Z

Trying to write some tests, but I'm having some trouble getting started. I discovered /test/helpers/config.php, and the only advice it gives is to run

phpunit Alltests.php --slow-tests

or

phpunit ????Test.php

But there is no file called AllTests.php in the project, and when I try to run a specific test, I get complaints about SnakeCase_PHPUnit_Framework being missing; where is the autoloader? Which directory am I supposed to be running this from? Is there a little tutorial or more detailed information somewhere, specifically for this project?

koenpunt · 2014-07-02T07:54:57Z

Try the following:

# install phpunit using composer
composer install
# run local version of phpunit
vendor/bin/phpunit ./test

koenpunt · 2014-07-02T08:06:58Z

Oh and if you want to use a different mysql/sqlite configuration for testing you can supply the connection strings as environment variables:

PHPAR_MYSQL=mysql://root@127.0.0.1:3306/phpar_test \
PHPAR_PGSQL=pgsql://root@127.0.0.1/phpar_test vendor/bin/phpunit ./test

shmax · 2014-07-02T17:08:57Z

@koenpunt Thanks much, that gets me unstuck.

shmax · 2014-07-04T21:59:01Z

@koenpunt
@cvanschalkwijk
Wrote unit tests.
Now using cache expire value from config, but you can override it by putting a $cache_expire static on your model subclass.

shmax · 2014-07-08T22:29:28Z

Not to be a pest, but can I ask what the plan is for merging some of these PRs? I see that mine is only one of many that have been waiting in line for quite some time. I only ask because there are a few other things I'd like to contribute (APC Cache adapter, version bumping for table schemas, bug fixes)...

andyleap · 2014-07-09T03:22:21Z

It's kinda a crap shoot, as it seems like most of the people that can push are largely inactive. IMHO, you'll be better off looking through PR's for features you need and pulling them into your own fork. That's what I'd do, but I've mostly moved on from PHP.

koenpunt · 2014-07-09T10:10:04Z

All those PR's have to be reviewed, updated, merged and tested, and that is a lot of work. And for me, I use AR in production extensively, but I'm not in need of the features proposed here on Github, so that makes that it's not a priority for me to get this merged. So from time to time I take stab at some PR's, but it's still very time consuming, also for the fact that there are more PR's submitted over time.

shmax · 2014-07-09T16:10:21Z

I'm using it in production as well (on two sites, one commercial), but that doesn't seem like the best justification to just sit on it. Like andyleap says, you can make your own fork and keep it in a state you like. Seems like a shame to just let the main branch stagnate, especially when so many of those PRs are legitimate fixes (personally, I'm waiting for the APC cache adapter, and fixes for foreign_key attributes of has_many associations, not to mention my own caching work, which has been promised on the main website for years) or valuable new features. Given that you guys are obviously overwhelmed--and who can blame you--and not really motivated to make changes, maybe it's time to grant merging privileges to a few new folks? I'm willing to help...

jpfuentes2 · 2014-07-09T16:18:58Z

The whitespace does not seem to be consistent in this file nor is it consistent with our poorly chosen style.

jpfuentes2 · 2014-07-09T16:19:31Z

lib/Cache.php

+     * Attempt to retrieve a value from cache using a key. If the value is not found, then the closure method
+     * will be invoked, and the result will be stored in cache using that key.
+     * @param $key
+     * @param $closure


Missing @param $expire

andyleap · 2014-07-09T16:22:56Z

Defaults have to be constant expressions, best bet would be to default to an invalid value, then check for that and replace with the option value in the function

jpfuentes2 · 2014-07-09T16:25:12Z

Defaults have to be constant expressions, best bet would be to default to an invalid value, then check for that and replace with the option value in the function

Forgive me: I haven't done PHP in a very very long time :). Agreed, maybe null and if so then use $options?

jpfuentes2 · 2014-07-09T16:25:53Z

lib/Cache.php


 		return $value;
 	}

+	public static function set($key, $var, $expire=0){
+		if (!static::$adapter)


I like the defensive programming but I don't think we do this elsewhere in this class. Is it necessary?

There are similar checks already in there; flush and get, to name two. I think you want to keep it--lessens the chance of something breaking for folks who haven't configured any cache adapters.

andyleap · 2014-07-09T16:26:32Z

lol, wasn't meant as an attack or anything, just run into trouble with that myself

jpfuentes2 · 2014-07-09T16:26:44Z

lib/Cache.php

+			return;
+
+		$key = static::get_namespace() . $key;
+


Empty line.

jpfuentes2 · 2014-07-09T16:30:41Z

lol, wasn't meant as an attack or anything, just run into trouble with that myself

No worries, didn't take it as such :)

jpfuentes2 · 2014-07-09T16:31:19Z

lib/Model.php

-		$list = static::table()->find($options);
+		$table = static::table();
+
+		if($table->cache_model){


I think I'd like to see the body of this conditional moved to a method for clarity of find_by_pk

shmax · 2014-07-09T17:57:44Z

Did my best to fix the whitespace issues, changed the cache_model attribute name as suggested, and changed the default expire value.

jpfuentes2 · 2014-07-10T13:40:39Z

Thanks for the updates! This is probably annoying at this point but we do have a certain style on this project -- terrible as it may be -- I'd like to maintain consistency. Can you fix {} to be on new lines?

The whitespace is still quite wonky. What are your editor settings like?

shmax · 2014-07-10T16:32:49Z

No worries, nitpick to your heart's delight, I take no offense. I've changed the brace style. By the way, speaking of braces, I'd like to suggest a new style rule: all if statements should use braces, even ones that are only followed by a single line. So:

if ($hasPuppy)
{
    echo "Aww, want to pet the puppy";
}

instead of

if ($hasPuppy)
    echo "Aww, want to pet the puppy";

I have a very practical justification for this suggestion: it's impossible to hit a breakpoint if the braces aren't there. ( http://bugs.xdebug.org/view.php?id=895 ) The debugger was enormously helpful to me in learning your code base, but I had to keep temporarily adding braces in order to make it workable (which may explain some of the wonky spacing; I also had tabs-as-spaces set in my IDE, which I've since changed).

I know it adds an extra line, which is why I long ago adopted the habit of putting the opening brace at the ends of lines.

koenpunt · 2014-07-10T19:22:29Z

lib/Cache.php

 		if (!static::$adapter)
 			return $closure();

 		if (!($value = static::$adapter->read($key)))
-			static::$adapter->write($key,($value = $closure()),static::$options['expire']);
+			static::$adapter->write($key,($value = $closure()),$expire);


As the talk on this PR is all about coding style, please be consistent with the spaces between arguments, so always add a space after a , (comma)

shmax · 2014-07-11T02:48:39Z

Added space before comma. Anything else?

koenpunt · 2014-07-11T11:42:33Z

Can you please trim down the number of commits by rebasing and merging related commits

koenpunt · 2014-07-11T11:43:26Z

And I agree on the style of if statements, they are better readable as well when having braces.

shmax · 2014-07-11T16:53:38Z

I just spent the whole morning trying to squash all those commits, and as you can see I only made things worse, just like last time. Everything looks fine on my local machine, but I have no idea how to clean up all the mess you see here. Do I have to close this PR and make a fresh one, or something?

koenpunt · 2014-07-11T17:15:47Z

You'll have to force push (git push origin implement-model-caching -f) to overwrite the remote branch.

shmax · 2014-07-12T09:38:34Z

Ah, that worked! Thanks much.

koenpunt · 2014-07-12T13:31:57Z

lib/Model.php

@@ -864,11 +878,26 @@ private function update($validate=true)
 			$dirty = $this->dirty_attributes();
 			static::table()->update($dirty,$pk);
 			$this->invoke_callback('after_update',false);
-		}
+            $this->update_cache();
+        }


The indenting here is still using spaces.

shmax · 2014-07-12T17:04:18Z

Fixed tabs, added newline at end of file.

koenpunt · 2014-07-13T14:21:54Z

I meant all files, so lib/Model.php, lib/Table.php, test/fixtures/publishers.csv and test/models/Publisher.php as well.

You can see which files do not have a newline by this icon:

shmax · 2014-07-13T15:59:55Z

Newlines restored.

koenpunt · 2014-07-13T16:43:17Z

test/CacheTest.php

@@ -21,7 +21,7 @@ public function tear_down()

 	private function cache_get()
 	{
-		return Cache::get("1337", function() { return "abcd"; });
+		return Cache::get("1337", function() { return "abcd"; },Cache::$options['expire']);


Is it necessary to pass the expire time to cache now? Then this could break current implementations. If not, please leave the change out here.

No, not necessary. Just being explicit, for clarity. Removed.

shmax · 2014-07-14T18:35:00Z

Removed semicolon.

jpfuentes2 · 2014-07-14T18:58:09Z

Thanks for putting up with us @shmax. I'm going to take another look today and concern myself more with the logic this time.

shmax · 2014-07-14T19:15:53Z

@jpfuentes2 No sweat, take all the time you need. I've already merged the code into my own fork (and have been using it quite happily on prod for weeks), so I'm not blocked, or anything.

koenpunt · 2014-07-14T19:42:34Z

@jpfuentes2 @shmax same for me

model caching

cvanschalkwijk · 2014-07-16T03:09:25Z

@shmax I like this a lot, thanks!

jpfuentes2 · 2014-07-16T13:11:14Z

Thanks @shmax.

filiplikavcan · 2014-07-16T14:27:57Z

lib/Table.php

@@ -367,7 +400,7 @@ private function get_meta_data()

 		$table_name = $this->get_fully_qualified_table_name($quote_name);
 		$conn = $this->conn;
-		$this->columns = Cache::get("get_meta_data-$table_name", function() use ($conn, $table_name) { return $conn->columns($table_name); });
+		$this->columns = Cache::get("get_meta_data-$table_name", function() use ($conn, $table_name) { return $conn->columns($table_name); }, Cache::$options['expire']);


After updating AR using Composer I'm getting not existing index "expire" notice. I fixed it by calling ActiveRecord\Cache::initialize(null); in ActiveRecord\Config::initialize callback. However this feature should not break existing installations which it currently does (with a strict error reporting mode).

This should be fixed in #421 (merged)

Looks like it. Good work @koenpunt!

@filer , can you pull down latest?

Thanks for the quick fix, guys, sorry 'bout that. I was always vaguely disturbed by the idea of a public static in Cache depending on 'initialize' being fired to be valid. What do you think about the idea of initializing it at declaration, instead?

cvanschalkwijk reviewed Jul 1, 2014
View reviewed changes

koenpunt mentioned this pull request Jul 2, 2014

update test instructions #418

Merged

jpfuentes2 reviewed Jul 9, 2014
View reviewed changes

lib/Cache.php

return;

$key = static::get_namespace() . $key;

Copy link

Owner

jpfuentes2 Jul 9, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty line.

jpfuentes2 reviewed Jul 9, 2014
View reviewed changes

koenpunt reviewed Jul 10, 2014
View reviewed changes

koenpunt reviewed Jul 12, 2014
View reviewed changes

koenpunt reviewed Jul 13, 2014
View reviewed changes

model caching

1ed2239

jpfuentes2 added a commit that referenced this pull request Jul 16, 2014

Merge pull request #417 from shmax/implement-model-caching

e3f2655

model caching

jpfuentes2 merged commit e3f2655 into jpfuentes2:master Jul 16, 2014

filiplikavcan reviewed Jul 16, 2014
View reviewed changes

model caching #417

model caching #417

Conversation

shmax commented Jul 1, 2014

koenpunt commented Jul 1, 2014

Choose a reason for hiding this comment

shmax commented Jul 2, 2014

koenpunt commented Jul 2, 2014

koenpunt commented Jul 2, 2014

shmax commented Jul 2, 2014

shmax commented Jul 4, 2014

shmax commented Jul 8, 2014

andyleap commented Jul 9, 2014

koenpunt commented Jul 9, 2014

shmax commented Jul 9, 2014

jpfuentes2 commented Jul 9, 2014

Choose a reason for hiding this comment

andyleap commented Jul 9, 2014

jpfuentes2 commented Jul 9, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyleap commented Jul 9, 2014

Choose a reason for hiding this comment

jpfuentes2 commented Jul 9, 2014

Choose a reason for hiding this comment

shmax commented Jul 9, 2014

jpfuentes2 commented Jul 10, 2014

shmax commented Jul 10, 2014

Choose a reason for hiding this comment

shmax commented Jul 11, 2014

koenpunt commented Jul 11, 2014

koenpunt commented Jul 11, 2014

shmax commented Jul 11, 2014

koenpunt commented Jul 11, 2014

shmax commented Jul 12, 2014

Choose a reason for hiding this comment

shmax commented Jul 12, 2014

koenpunt commented Jul 13, 2014

shmax commented Jul 13, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shmax commented Jul 14, 2014

jpfuentes2 commented Jul 14, 2014

shmax commented Jul 14, 2014

koenpunt commented Jul 14, 2014

cvanschalkwijk commented Jul 16, 2014

jpfuentes2 commented Jul 16, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment