This change has been marked as submitted.

Describe the submission (optional):

Merged into master

Reviews
Diff

- Download Diff

Summary:

Cache: cache

Review Request #528 — Created Aug. 14, 2017 and submitted 7 years, 9 months ago

Information
Owner:	p.valk*
Repository:	Lunr
Branch:	cache2
Bugs:
Depends On:
Commit:	2a326cb...
Reviewers
Groups:	lunr
People:

Description

psuedocode example of how to implement it inside a controller.
public function foo()
    {
            $key = 'some unique identifier';

            $cache = new cacheprovider(...);

            $values = $cache->get($key);

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

                $cache->set($key, $values);
            }
        }

        // Return data.
        return $values;
    }

Testing Done

Issues

0
0
38
0
All Issues: 38

Description	From	Last Updated
unrelated to this review	pprkut	7 years, 9 months ago
Incomplete namespace declaration for params	pprkut	7 years, 9 months ago
why do we need this at all?	pprkut	7 years, 9 months ago
missing phpdoc	pprkut	7 years, 9 months ago
missing phpdoc	pprkut	7 years, 9 months ago
I don't see an isMiss() method in PSR-6	pprkut	7 years, 9 months ago
Why do we need to do this here? It has nothing to do with getting data from the cache.	pprkut	7 years, 9 months ago
Incomplete documentation Order of params doesn't match implementation	pprkut	7 years, 9 months ago
I don't know what you mean with singleton, but this class should be completely reentrant. I should be able to ...	pprkut	7 years, 9 months ago
no TTL in $config that's just too messy	pprkut	7 years, 9 months ago
unrelated to this review	pprkut	7 years, 9 months ago
should be up with the other php libs	pprkut	7 years, 9 months ago
protected $pool	pprkut	7 years, 9 months ago
not used anymore	pprkut	7 years, 9 months ago
"get" is enough. This will most likely be $cache->get(), so $cache->get_cache() would just have useless information.	pprkut	7 years, 9 months ago
superfluous comments	pprkut	7 years, 9 months ago
time to live	pprkut	7 years, 9 months ago
double bool	pprkut	7 years, 9 months ago
set() is enough a 30 second default is rather short. That's fine for really high traffic sites, but for low ...	pprkut	7 years, 9 months ago
superfluous comment	pprkut	7 years, 9 months ago
please don't call it a controller. Controller methods are accessible via web/cli, which these clearly shouldn't be.	pprkut	7 years, 9 months ago
formatting	pprkut	7 years, 9 months ago
doesn't exist anymore	pprkut	7 years, 9 months ago
Incomplete phpdoc	pprkut	7 years, 9 months ago
you can do $item->get() inside the if statement	pprkut	7 years, 9 months ago
Incomplete phpdoc	pprkut	7 years, 9 months ago
How can the parameter be int and still need to be cast to Int? Don't accomodate for people refusing to ...	smillernl	7 years, 9 months ago
I think it's easier to follow the code if you do this in two steps	pprkut	7 years, 9 months ago
Naming here follows the namespace naming (making this wrong)	smillernl	7 years, 9 months ago
Not required right?	smillernl	7 years, 9 months ago
This should be a type, not a variable name	pprkut	7 years, 9 months ago
Full or relative namespace please	pprkut	7 years, 9 months ago
We're not actually requiring any Stash specifics here, so we should depend on the PSR-6 interface only	pprkut	7 years, 9 months ago
we don't need stash anymore, only psr-6. Projects may choose stash as a psr-6 implementation	pprkut	7 years, 9 months ago
see below	smillernl	7 years, 9 months ago
See below	smillernl	7 years, 9 months ago
This should be Psr, can't have tests determine implementation	smillernl	7 years, 9 months ago
Nope	smillernl	7 years, 9 months ago
There are no open issues

Commit:

-	dc71ef545ebc7525ef3254dc9842847b299ac95f
+	3bce5b23d9089106e13c06b69d7a1e986b556055

Diff:

Revision 2 (+211)

Show changes

	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)

The issue has been resolved. Show all issues

unrelated to this review

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

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

The issue has been resolved. Show all issues

why do we need this at all?

p.valk 7 years, 9 months ago

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

pprkut 7 years, 9 months ago

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)
The issue has been resolved. Show all issues
```
missing phpdoc
```
src/Lunr/Gravity/CacheController.php (Diff revision 2)
The issue has been resolved. Show all issues
```
missing phpdoc
```
src/Lunr/Gravity/CacheController.php (Diff revision 2)
The issue has been resolved. Show all issues
```
I don't see an isMiss() method in PSR-6
```

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

The issue has been resolved. Show all issues

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

p.valk 7 years, 9 months ago

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

pprkut 7 years, 9 months ago

why do we need it in a locking function?

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

The issue has been resolved. Show all issues

Incomplete documentation
Order of params doesn't match implementation

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

The issue has been resolved. 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.

p.valk 7 years, 9 months ago

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

pprkut 7 years, 9 months ago

Not following. Why do we need locking here?

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

The issue has been resolved. Show all issues

no TTL in $config

that's just too messy

p.valk 7 years, 9 months ago

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

pprkut 7 years, 9 months ago

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)
The issue has been resolved. 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)

Show changes

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

composer.json (Diff revision 3)
The issue has been resolved. Show all issues
```
should be up with the other php libs
```
src/Lunr/Gravity/CacheController.php (Diff revision 3)
The issue has been resolved. Show all issues
```
protected $pool
```
src/Lunr/Gravity/CacheController.php (Diff revision 3)
The issue has been resolved. Show all issues
```
not used anymore
```

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

The issue has been resolved. 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)

The issue has been resolved. Show all issues

superfluous comments

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

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

The issue has been resolved. 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)
The issue has been resolved. Show all issues
```
superfluous comment
```

Commit:

-	09b5828892d077692e5a0ae3113536f9adfff53e
+	388193b580d8d30c28bcd432ac805a70b9eaab33

Diff:

Revision 4 (+226 -2)

Show changes

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

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

The issue has been resolved. 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)
The issue has been resolved. Show all issues
```
formatting
```
src/Lunr/Gravity/CacheController.php (Diff revision 4)
The issue has been resolved. Show all issues
```
doesn't exist anymore
```
src/Lunr/Gravity/CacheController.php (Diff revision 4)
The issue has been resolved. Show all issues
```
Incomplete phpdoc
```

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

The issue has been resolved. Show all issues

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

p.valk 7 years, 9 months ago

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 7 years, 9 months ago

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

p.valk 7 years, 9 months ago

nevermind my point above, confused 2 variables.

src/Lunr/Gravity/CacheController.php (Diff revision 4)
The issue has been resolved. Show all issues
```
Incomplete phpdoc
```
src/Lunr/Gravity/CacheController.php (Diff revision 4)
The issue has been resolved. 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)

The issue has been resolved. 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 7 years, 9 months ago
```
ok! 
```

tests/test.bootstrap.inc.php (Diff revision 4)
The issue has been resolved. Show all issues
```
Naming here follows the namespace naming (making this wrong)
```

Commit:

-	388193b580d8d30c28bcd432ac805a70b9eaab33
+	c98be3f59be4ba3aba00dc4785b6f595a0510e4f

Diff:

Revision 5 (+515 -2)

Show changes

	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)
The issue has been resolved. Show all issues
```
Not required right?
```

Commit:

-	c98be3f59be4ba3aba00dc4785b6f595a0510e4f
+	d73315d24dccf529d310619b7cd67d1b5f0948a0

Diff:

Revision 6 (+514 -2)

Show changes

	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)
The issue has been resolved. Show all issues
```
This should be a type, not a variable name
```

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

The issue has been resolved. 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)

Show changes

	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)
The issue has been resolved. Show all issues
```
Full or relative namespace please
```

composer.json (Diff revision 7)

The issue has been resolved. Show all issues

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

p.valk 7 years, 9 months ago

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

p.valk 7 years, 9 months ago
```
i meant decomposer*
```
pprkut 7 years, 9 months ago
```
yes to both
```

Commit:

-	202ec649e5c53ab989739cc9a1817efebeda52c5
+	f88ec412be944b3df023375ca22e98ef1b39df26

Diff:

Revision 8 (+443 -2)

Show changes

	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)
The issue has been resolved. Show all issues
```
see below
```
src/Lunr/Cache/Tests/CacheHandlerDataTest.php (Diff revision 8)
The issue has been resolved. Show all issues
```
See below
```
src/Lunr/Cache/Tests/CacheHandlerTest.php (Diff revision 8)
The issue has been resolved. Show all issues
```
This should be Psr, can't have tests determine implementation
```
tests/test.bootstrap.inc.php (Diff revision 8)
The issue has been resolved. Show all issues
```
Nope
```

Commit:

-	f88ec412be944b3df023375ca22e98ef1b39df26
+	2a326cbfe394935f124d2f29ada10391799138c1

Diff:

Revision 9 (+443 -2)

Show changes

	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

Status: Closed (submitted)

Change Summary:

Merged into master

You have a pending review.

Review Board 3.0.24

This change has been marked as submitted.

Cache: cache

Commit:

Diff:

Description:

Description:

Change Summary:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Branch:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Description:

Commit:

Diff:

Status: Closed (submitted)

Change Summary: