You have a pending review.

	decomposer.json
	src/Lunr/Gravity/CacheController.php
	tests/test.bootstrap.inc.php

Description:

~		cache
	~	example of implementation in texture HomeController.php
	+
	+	public function index()
	+	{
	+	$locale = $this->locator->locale()->get_locale();
	+
	+	$api = $this->locator->contentfulapi();
	+
	+	$api->access_token = $this->config['contentful']['website']['access_token'];
	+
	+	$api->set_space_id($this->config['contentful']['website']['id']);
	+
	+	foreach(['projects', 'customers', 'post', 'jobs'] as $type)
	+	{
	+	$filter = [];
	+	$filter['locale'] = str_replace('_', '-', $locale);
	+
	+	if ($type == 'post')
	+	{
	+	$filter['order'] = '-fields.date';
	+	$filter['fields.hidden[ne]'] = 1;
	+	$filter['limit'] = 3;
	+	$filter['fields.' . $locale] = true;
	+	}
	+	if ($type == 'projects')
	+	{
	+	$filter['order'] = '-fields.featured,-fields.date';
	+	$filter['fields.hidden[ne]'] = 1;
	+	$filter['limit'] = 6;
	+	$filter['fields.' . $locale] = true;
	+	}
	+	if ($type == 'jobs')
	+	{
	+	$filter['order'] = '-fields.date';
	+	$filter['fields.hidden[ne]'] = 1;
	+	$filter['fields.' . $locale] = true;
	+	}
	+
	+	$cache = $this->locator->cacheprovider();
	+
	+	// to make sure the key is the same even when the order would be different
	+	$filters = ksort($filter);
	+
	+	$values = $cache->get('HomeController/Index', $type . '/' . implode('/', $filters));
	+
	+	if ($values === FALSE)
	+	{
	+	$values = $api->get_entries($type, $filter);
	+
	+	$cache->set('HomeController/Index', $type . '/' . implode('/', $filters), $values);
	+
	+	$this->response->add_response_data($type, $values);
	+	}
	+	else
	+	{
	+	$this->response->add_response_data($type, $values);
	+	}
	+	}
	+
	+	$this->set_result(HttpCode::OK);
	+	}

Description:

~		example of implementation in texture HomeController.php
	~	psuedocode example of how to implement it inside a controller.

~		public function index()
	~	public function foo()
		{
~		$locale = $this->locator->locale()->get_locale();
	~	$key = 'some unique identifier';

~		$api = $this->locator->contentfulapi();
	~	$cache = new cacheprovider(...);

~		$api->access_token = $this->config['contentful']['website']['access_token'];
	~	$values = $cache->get('classname/methodname', $key);
-
-		$api->set_space_id($this->config['contentful']['website']['id']);
-
-		foreach(['projects', 'customers', 'post', 'jobs'] as $type)
-		{
-		$filter = [];
-		$filter['locale'] = str_replace('_', '-', $locale);
-
-		if ($type == 'post')
-		{
-		$filter['order'] = '-fields.date';
-		$filter['fields.hidden[ne]'] = 1;
-		$filter['limit'] = 3;
-		$filter['fields.' . $locale] = true;
-		}
-		if ($type == 'projects')
-		{
-		$filter['order'] = '-fields.featured,-fields.date';
-		$filter['fields.hidden[ne]'] = 1;
-		$filter['limit'] = 6;
-		$filter['fields.' . $locale] = true;
-		}
-		if ($type == 'jobs')
-		{
-		$filter['order'] = '-fields.date';
-		$filter['fields.hidden[ne]'] = 1;
-		$filter['fields.' . $locale] = true;
-		}
-
-		$cache = $this->locator->cacheprovider();
-
-		// to make sure the key is the same even when the order would be different
-		$filters = ksort($filter);
-
-		$values = $cache->get('HomeController/Index', $type . '/' . implode('/', $filters));

		if ($values === FALSE)
		{
		$values = $api->get_entries($type, $filter);

~		$cache->set('HomeController/Index', $type . '/' . implode('/', $filters), $values);
	~	$cache->set('classname/methodname', $key, $values);
-
-		$this->response->add_response_data($type, $values);
-		}
-		else
-		{
-		$this->response->add_response_data($type, $values);
		}
		}

~		$this->set_result(HttpCode::OK);
	~	// Return data.
	+	return $values;
		}

```
Missing the composer changes
```

decomposer.json (Diff revision 2)

Show all issues

unrelated to this review

src/Lunr/Gravity/CacheController.php (Diff revision 2)
Show all issues
```
Incomplete namespace declaration for params
```

src/Lunr/Gravity/CacheController.php (Diff revision 2)

Show all issues

why do we need this at all?

looks cleaner and to make sure the key is the same in every function.

for the record, single line private/protected functions are almost never worth it.

It might be worth it here, but then you better have a very good argument for it

src/Lunr/Gravity/CacheController.php (Diff revision 2)
Show all issues
```
missing phpdoc
```
src/Lunr/Gravity/CacheController.php (Diff revision 2)
Show all issues
```
missing phpdoc
```
src/Lunr/Gravity/CacheController.php (Diff revision 2)
Show all issues
```
I don't see an isMiss() method in PSR-6
```

src/Lunr/Gravity/CacheController.php (Diff revision 2)

Show all issues

Why do we need to do this here? It has nothing to do with getting data from the cache.

to make the implementation as easy as possible.
i'l move it to a locking function.

why do we need it in a locking function?

src/Lunr/Gravity/CacheController.php (Diff revision 2)

Show all issues

Incomplete documentation
Order of params doesn't match implementation

src/Lunr/Gravity/CacheController.php (Diff revision 2)

Show all issues

I don't know what you mean with singleton, but this class should be completely reentrant. I should be able to call get() and set() in any order with any keys and it should work without causing side effects.

ok, well then the implementation would need an extra call to lock the item while it is refreshing.

Not following. Why do we need locking here?

src/Lunr/Gravity/CacheController.php (Diff revision 2)

Show all issues

no TTL in $config

that's just too messy

i tought you would prefer this over hardcoded, i can let it be a parameter to the set_cache function

If you put this in a config file, you'd end up with a config file with a couple hundred entries (in large applications).

tests/test.bootstrap.inc.php (Diff revision 2)
Show all issues
```
unrelated to this review
```

Change Summary:

we would need to decidde on a default ttl value.

Commit:

-	3bce5b23d9089106e13c06b69d7a1e986b556055
+	09b5828892d077692e5a0ae3113536f9adfff53e

Diff:

Revision 3 (+132 -1)

	composer.json
	decomposer.json
	src/Lunr/Gravity/CacheController.php
	tests/test.bootstrap.inc.php

composer.json (Diff revision 3)

Show all issues

should be up with the other php libs

src/Lunr/Gravity/CacheController.php (Diff revision 3)
Show all issues
```
protected $pool
```
src/Lunr/Gravity/CacheController.php (Diff revision 3)
Show all issues
```
not used anymore
```

src/Lunr/Gravity/CacheController.php (Diff revision 3)

Show all issues

"get" is enough.
This will most likely be $cache->get(), so $cache->get_cache() would just have useless information.

src/Lunr/Gravity/CacheController.php (Diff revision 3)

Show all issues

superfluous comments

src/Lunr/Gravity/CacheController.php (Diff revision 3)
Show all issues
```
time to live
```
src/Lunr/Gravity/CacheController.php (Diff revision 3)
Show all issues
```
double bool
```

src/Lunr/Gravity/CacheController.php (Diff revision 3)

Show all issues

set() is enough

a 30 second default is rather short. That's fine for really high traffic sites, but for low traffic sites that just means there won't ever be a cache hit. Make it like 5 or 10 minutes.

src/Lunr/Gravity/CacheController.php (Diff revision 3)
Show all issues
```
superfluous comment
```

Commit:

-	09b5828892d077692e5a0ae3113536f9adfff53e
+	388193b580d8d30c28bcd432ac805a70b9eaab33

Diff:

Revision 4 (+226 -2)

	composer.lock
	composer.json
	decomposer.json
	src/Lunr/Gravity/CacheController.php
	tests/test.bootstrap.inc.php

src/Lunr/Gravity/CacheController.php (Diff revision 4)

Show all issues

please don't call it a controller. Controller methods are accessible via web/cli, which these clearly shouldn't be.

src/Lunr/Gravity/CacheController.php (Diff revision 4)
Show all issues
```
formatting
```
src/Lunr/Gravity/CacheController.php (Diff revision 4)
Show all issues
```
doesn't exist anymore
```
src/Lunr/Gravity/CacheController.php (Diff revision 4)
Show all issues
```
Incomplete phpdoc
```

src/Lunr/Gravity/CacheController.php (Diff revision 4)

Show all issues

you can do $item->get() inside the if statement

p.valk Aug. 16, 2017, 3:05 p.m.

i prefer doing it like this, looks cleaner. and you need to do ->isHit() anyway.
or if you want smaller code change the if to return $item->isHit() === true ? $data : false;

pprkut Aug. 16, 2017, 3:48 p.m.

It's not about smaller code, it's about not storing useless data.

p.valk Aug. 16, 2017, 3:53 p.m.

nevermind my point above, confused 2 variables.

src/Lunr/Gravity/CacheController.php (Diff revision 4)
Show all issues
```
Incomplete phpdoc
```

src/Lunr/Gravity/CacheController.php (Diff revision 4)

Show all issues

I think it's easier to follow the code if you do this in two steps

src/Lunr/Gravity/CacheController.php (Diff revision 4)

Show all issues

How can the parameter be int and still need to be cast to Int? Don't accomodate for people refusing to follow the rules.

p.valk Aug. 16, 2017, 3:46 p.m.
```
ok! 
```

tests/test.bootstrap.inc.php (Diff revision 4)

Show all issues

Naming here follows the namespace naming (making this wrong)

Commit:

-	388193b580d8d30c28bcd432ac805a70b9eaab33
+	c98be3f59be4ba3aba00dc4785b6f595a0510e4f

Diff:

Revision 5 (+515 -2)

	composer.lock
	composer.json
	decomposer.json
	src/Lunr/Cache/Cache.php
	src/Lunr/Cache/Tests/CacheHandlerBaseTest.php
	src/Lunr/Cache/Tests/CacheHandlerDataTest.php
	src/Lunr/Cache/Tests/CacheHandlerTest.php
	tests/phpunit.xml
	1 more

Branch:

+	cache2

composer.json (Diff revision 5)
Show all issues
```
Not required right?
```

Commit:

-	c98be3f59be4ba3aba00dc4785b6f595a0510e4f
+	d73315d24dccf529d310619b7cd67d1b5f0948a0

Diff:

Revision 6 (+514 -2)

	composer.lock
	composer.json
	decomposer.json
	src/Lunr/Cache/Cache.php
	src/Lunr/Cache/Tests/CacheHandlerBaseTest.php
	src/Lunr/Cache/Tests/CacheHandlerDataTest.php
	src/Lunr/Cache/Tests/CacheHandlerTest.php
	tests/phpunit.xml
	1 more

src/Lunr/Cache/Cache.php (Diff revision 6)
Show all issues
```
This should be a type, not a variable name
```

src/Lunr/Cache/Cache.php (Diff revision 6)

Show all issues

We're not actually requiring any Stash specifics here, so we should depend on the PSR-6 interface only

Commit:

-	d73315d24dccf529d310619b7cd67d1b5f0948a0
+	202ec649e5c53ab989739cc9a1817efebeda52c5

Diff:

Revision 7 (+514 -2)

	composer.lock
	composer.json
	decomposer.json
	src/Lunr/Cache/Cache.php
	src/Lunr/Cache/Tests/CacheHandlerBaseTest.php
	src/Lunr/Cache/Tests/CacheHandlerDataTest.php
	src/Lunr/Cache/Tests/CacheHandlerTest.php
	tests/phpunit.xml
	1 more

Ship it!

src/Lunr/Cache/Cache.php (Diff revisions 6 - 7)
Show all issues
```
Full or relative namespace please
```

composer.json (Diff revision 7)

Show all issues

we don't need stash anymore, only psr-6.
Projects may choose stash as a psr-6 implementation

p.valk Aug. 17, 2017, 10:04 a.m.

so also remove it from composer, then only include the psr/cache

p.valk Aug. 17, 2017, 10:14 a.m.
```
i meant decomposer*
```
pprkut Aug. 17, 2017, 10:16 a.m.
```
yes to both
```

Commit:

-	202ec649e5c53ab989739cc9a1817efebeda52c5
+	f88ec412be944b3df023375ca22e98ef1b39df26

Diff:

Revision 8 (+443 -2)

	composer.lock
	composer.json
	decomposer.json
	src/Lunr/Cache/Cache.php
	src/Lunr/Cache/Tests/CacheHandlerBaseTest.php
	src/Lunr/Cache/Tests/CacheHandlerDataTest.php
	src/Lunr/Cache/Tests/CacheHandlerTest.php
	tests/phpunit.xml
	1 more

Description:

		psuedocode example of how to implement it inside a controller.

		public function foo()
		{
		$key = 'some unique identifier';

		$cache = new cacheprovider(...);

~		$values = $cache->get('classname/methodname', $key);
	~	$values = $cache->get($key);

		if ($values === FALSE)
		{
		$values = $api->get_entries($type, $filter);

~		$cache->set('classname/methodname', $key, $values);
	~	$cache->set($key, $values);
		}
		}

		// Return data.
		return $values;
		}

src/Lunr/Cache/Tests/CacheHandlerDataTest.php (Diff revision 8)
Show all issues
```
see below
```
src/Lunr/Cache/Tests/CacheHandlerDataTest.php (Diff revision 8)
Show all issues
```
See below
```
src/Lunr/Cache/Tests/CacheHandlerTest.php (Diff revision 8)
Show all issues
```
This should be Psr, can't have tests determine implementation
```
tests/test.bootstrap.inc.php (Diff revision 8)
Show all issues
```
Nope
```

Commit:

-	f88ec412be944b3df023375ca22e98ef1b39df26
+	2a326cbfe394935f124d2f29ada10391799138c1

Diff:

Revision 9 (+443 -2)