Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[Cache] Add CacheItem::validateKey utility method #18597

New issue

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 20, 2016
Merged

[Cache] Add CacheItem::validateKey utility method #18597

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 3 additions & 14 deletions 17 src/Symfony/Component/Cache/Adapter/AbstractAdapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
use Psr\Log\LoggerAwareInterface;
use Psr\Log\LoggerAwareTrait;
use Symfony\Component\Cache\CacheItem;
use Symfony\Component\Cache\Exception\InvalidArgumentException;

/**
* @author Nicolas Grekas <p@tchwork.com>
Expand All @@ -31,7 +30,7 @@ abstract class AbstractAdapter implements AdapterInterface, LoggerAwareInterface

protected function __construct($namespace = '', $defaultLifetime = 0)
{
$this->namespace = $this->getId($namespace, true);
$this->namespace = '' === $namespace ? '' : $this->getId($namespace);
$this->createCacheItem = \Closure::bind(
function ($key, $value, $isHit) use ($defaultLifetime) {
$item = new CacheItem();
Expand Down Expand Up @@ -336,19 +335,9 @@ public function __destruct()
}
}

private function getId($key, $ns = false)
private function getId($key)
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for adding comments in a merged PR, but this was merged a bit quickly. I have some concerns about how validateKey() is used. It's a validator, but it's used like a getter or a normalizer.

Instead of this:

private function getId($key)
{
    return $this->namespace.CacheItem::validateKey($key);
}

Why not using it like this?

private function getId($key)
{
    CacheItem::validateKey($key);

    return $this->namespace.$key;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This would just raise the boilerplate, whereas the current behavior keep it low IMHO.

{
if (!is_string($key)) {
throw new InvalidArgumentException(sprintf('Cache key must be string, "%s" given', is_object($key) ? get_class($key) : gettype($key)));
}
if (!isset($key[0]) && !$ns) {
throw new InvalidArgumentException('Cache key length must be greater than zero');
}
if (isset($key[strcspn($key, '{}()/\@:')])) {
throw new InvalidArgumentException('Cache key contains reserved characters {}()/\@:');
}

return $this->namespace.$key;
return $this->namespace.CacheItem::validateKey($key);
}

private function generateItems($items, &$keys)
Expand Down
22 changes: 3 additions & 19 deletions 22 src/Symfony/Component/Cache/Adapter/ArrayAdapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
use Psr\Log\LoggerAwareInterface;
use Psr\Log\LoggerAwareTrait;
use Symfony\Component\Cache\CacheItem;
use Symfony\Component\Cache\Exception\InvalidArgumentException;

/**
* @author Nicolas Grekas <p@tchwork.com>
Expand Down Expand Up @@ -74,7 +73,7 @@ public function getItem($key)
public function getItems(array $keys = array())
{
foreach ($keys as $key) {
$this->validateKey($key);
CacheItem::validateKey($key);
}

return $this->generateItems($keys, time());
Expand All @@ -85,7 +84,7 @@ public function getItems(array $keys = array())
*/
public function hasItem($key)
{
return isset($this->expiries[$this->validateKey($key)]) && ($this->expiries[$key] >= time() || !$this->deleteItem($key));
return isset($this->expiries[CacheItem::validateKey($key)]) && ($this->expiries[$key] >= time() || !$this->deleteItem($key));
}

/**
Expand All @@ -103,7 +102,7 @@ public function clear()
*/
public function deleteItem($key)
{
unset($this->values[$this->validateKey($key)], $this->expiries[$key]);
unset($this->values[CacheItem::validateKey($key)], $this->expiries[$key]);

return true;
}
Expand Down Expand Up @@ -169,21 +168,6 @@ public function commit()
return true;
}

private function validateKey($key)
{
if (!is_string($key)) {
throw new InvalidArgumentException(sprintf('Cache key must be string, "%s" given', is_object($key) ? get_class($key) : gettype($key)));
}
if (!isset($key[0])) {
throw new InvalidArgumentException('Cache key length must be greater than zero');
}
if (isset($key[strcspn($key, '{}()/\@:')])) {
throw new InvalidArgumentException('Cache key contains reserved characters {}()/\@:');
}

return $key;
}

private function generateItems(array $keys, $now)
{
$f = $this->createCacheItem;
Expand Down
17 changes: 3 additions & 14 deletions 17 src/Symfony/Component/Cache/Adapter/ProxyAdapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
use Psr\Cache\CacheItemInterface;
use Psr\Cache\CacheItemPoolInterface;
use Symfony\Component\Cache\CacheItem;
use Symfony\Component\Cache\Exception\InvalidArgumentException;

/**
* @author Nicolas Grekas <p@tchwork.com>
Expand All @@ -31,7 +30,7 @@ class ProxyAdapter implements AdapterInterface
public function __construct(CacheItemPoolInterface $pool, $namespace = '', $defaultLifetime = 0)
{
$this->pool = $pool;
$this->namespace = $this->getId($namespace, true);
$this->namespace = '' === $namespace ? '' : $this->getId($namespace);
$this->namespaceLen = strlen($namespace);
$this->createCacheItem = \Closure::bind(
function ($key, $value, $isHit) use ($defaultLifetime) {
Expand Down Expand Up @@ -192,18 +191,8 @@ public function getMisses()
return $this->misses;
}

private function getId($key, $ns = false)
private function getId($key)
{
if (!is_string($key)) {
throw new InvalidArgumentException(sprintf('Cache key must be string, "%s" given', is_object($key) ? get_class($key) : gettype($key)));
}
if (!isset($key[0]) && !$ns) {
throw new InvalidArgumentException('Cache key length must be greater than zero');
}
if (isset($key[strcspn($key, '{}()/\@:')])) {
throw new InvalidArgumentException('Cache key contains reserved characters {}()/\@:');
}

return $this->namespace.$key;
return $this->namespace.CacheItem::validateKey($key);
}
}
24 changes: 24 additions & 0 deletions 24 src/Symfony/Component/Cache/CacheItem.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,30 @@ public function expiresAfter($time)
return $this;
}

/**
* Validates a cache key according to PSR-6.
*
* @param string $key The key to validate.
*
* @return string $key if it is valid.
*
* @throws InvalidArgumentException When $key is not valid.
*/
public static function validateKey($key)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can refactor this into isValidKey() and return a boolean instead of the key.

Copy link
Member

Choose a reason for hiding this comment

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

returning a boolean would mean that it should not throw the exception either, which would complicate things a lot (especially to give the right error message). So this looks like a bad idea to me.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this comment. We can return a boolean and throw exceptions. Some Symfony methods already do that.

Anyway, maybe the problem is with the method name, because it's not a validator but a normalizer (in theory it could change the value of the key to make it valid).

Copy link
Member Author

Choose a reason for hiding this comment

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

A normalizer would transform its argument, here the argument is only checked against a set of rules and never modified. To me, it validates...

Copy link
Member

@javiereguiluz javiereguiluz Apr 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I understand a validator as something that returns true/false depending whether the value is valid or not. But here we are throwing exceptions and returning the original value (potentially modified). To me it's very confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's exactly the helper that PSR-6 implementations require, only that. Not some sort of architectural master piece...

potentially modified

Never!

Copy link
Member

@javiereguiluz javiereguiluz Apr 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Never!

In theory. But since we return the value, we could modify it unawarely (10 minutes ago you fixed a similar problem: #18603). This couldn't happen with a proper true/false validator.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is very well tested. Try changing it and everything will go red, really.

Copy link
Member

Choose a reason for hiding this comment

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

@javiereguiluz but your suggestion of a true/false validator is precisely what makes things much harder to use: when receiving false, you cannot know what is the reason, and so you cannot put a meaningful message in your exception. And if you keep throwing inside the validator, it is not a true/false one anymore but a true/Exception one (with a name isValid which still make people think it is a true/false one). In this case, I would argue that a void/Exception signature is better than a true/Exception one.

Copy link
Member Author

Choose a reason for hiding this comment

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

See #18604

{
if (!is_string($key)) {
throw new InvalidArgumentException(sprintf('Cache key must be string, "%s" given', is_object($key) ? get_class($key) : gettype($key)));
}
if (!isset($key[0])) {
throw new InvalidArgumentException('Cache key length must be greater than zero');
}
if (isset($key[strcspn($key, '{}()/\@:')])) {
throw new InvalidArgumentException('Cache key contains reserved characters {}()/\@:');
}

return $key;
}

/**
* Internal logging helper.
*
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.