[RFC][WIP][2.3] Cache Component #5902

Closed
wants to merge 1 commit into
from

10 participants

@dlsniper

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #1513, #3211
License of the code: MIT
Documentation PR: ~
Todo:

  • feedback from GH;
  • implement more features for namespaces;
  • implement a tagging mechanism;
  • add more tests;
  • implement more caching drivers;
  • add create documentation PR for the component;
  • fix any CS break;
  • optimizations?

Hello,

This is an attempt to implement a much needed component in Symfony, hence the Request For Comments status.

After reading and the thinking about this for the past weeks and knowing the previous discussions about the possible implementation of such a feature I've decided to make something as simple as possible while still allowing for flexibility to extend it as one might see fit.

One could argue that I'm missing cache namespaces, caching locking and so on, but those are subject to discussion / implementation.

I'll try and add the remaining things from the TODO list in the specified order.

Also keep in mind that this is only the component, I've split the bundle into a separate PR to maintain the feedback separated., see #5903. Also, if it's decided, I'll create a new PR for the implementation of the component in other components/places.

At least for now I don't care about coding style breaks and so on, I think it's the least important thing right now (don't worry, I'll fix everything, if anything, before merging), so please don't comment about it!

As a last thing, I really want to finish this in time for 2.2 release/feature freeze so if you want to help me out, any and all PRs are welcomed.

Thank you!

@stof
Symfony member

This does not fixes #3211 at all as it does not take this discussion into account at all.

@dlsniper

Hi @stof it's a WIP for the moment, I have taken the talk into consideration but since the original author of it doesn't have time nor anyone else jumped with PRs there or meanwhile, it will take a while to take all that into consideration and write all the code around that. Now that I have the bundle working the rest of the component should be done faster as I wanted first to have the user perspective into account as well.

@stof stof and 1 other commented on an outdated diff Nov 3, 2012
src/Symfony/Component/Cache/Cache.php
+ {
+ $driver = strtolower($driver);
+
+ if (!isset($this->drivers[$driver])) {
+ return false;
+ }
+
+ if ($instanceName == '') {
+ $this->instances[$driver][] = new $this->drivers[$driver]($configuration);
+ end($this->instances[$driver]);
+
+ $result = key($this->instances[$driver]);
+ } else {
+ $this->instances[$driver][$instanceName] = new $this->drivers[$driver]($configuration);
+
+ $result = $this->instances[$driver][$instanceName];
@stof
Symfony member
stof added a line comment Nov 3, 2012

This would make it return adriver instance, not an integer or a boolean

@dlsniper
dlsniper added a line comment Nov 3, 2012

I'm not sure who to make this as if you want the cache to be managed by the code then you need to get the cache instance but if you already know your instance name then there's no point in returning it, right? Or should I make it return it just for the sake of consistency?

@stof
Symfony member
stof added a line comment Nov 3, 2012

The return value of a method should be consistent. Otherwise, it makes it really difficult to use it. Creating a crappy interface is a bad idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 3, 2012
src/Symfony/Component/Cache/Cache.php
+
+ /**
+ * Remove a certain instance from the current instances
+ *
+ * @param string $driver
+ * @param int $instanceId
+ *
+ * @return bool|Cache
+ */
+ public function despawnDriverInstance($driver, $instanceId)
+ {
+ $driver = strtolower($driver);
+ $instanceId = (int)$instanceId;
+
+ if (!isset($this->drivers[$driver]) || !(isset($this->instances[$driver][$instanceId]))) {
+ return false;
@stof
Symfony member
stof added a line comment Nov 3, 2012

why whould it return false here when the other case provides a fluent interface ? It does not make sense

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Nov 3, 2012
...fony/Component/Cache/Drivers/CacheDriverInterface.php
+ * @return boolean
+ */
+ function remove($key);
+
+ /**
+ * Perform any changes to the driver before unsetting it from PHP
+ *
+ * @return bool
+ */
+ function despawn();
+
+ /**
+ * Get the raw driver in order to enable direct commands
+ * @return mixed
+ */
+ function getRawDriver();
@stof
Symfony member
stof added a line comment Nov 3, 2012

why would you have such a method in the driver interface ?

@dlsniper
dlsniper added a line comment Nov 3, 2012

I've added this because people might want to send raw commands to the caching driver much like you can to the with Doctrine to send raw SQL.
It it written into the interface so that you always have access to the driver, no matter what implementation you have.

If it's not deemed useful, I'll remove it from the interface.

@stof
Symfony member
stof added a line comment Nov 3, 2012

This method is implementation specific as its return value is specific to each implementation. So adding it in the interface does not provide any value IMO. Using it already ties your code to a specific implementation, and it forces to implement such method even when there is no raw driver (see your ApcCache)

@dlsniper
dlsniper added a line comment Nov 3, 2012

Ok, I think I see your point but then the problem is that even when using Doctrine Common as provider, you'll still need to implement a bridge for it as there's no PSR for a interface nor a most common way of naming the functions. Once a PSR will appear then I'll be able to use it to standardize the interface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Nov 3, 2012
src/Symfony/Component/Cache/Cache.php
+
+ // @TODO Add some detection for the class to implement the CacheDriverInterface
+
+ $this->drivers[$driverName] = $driverClass;
+ $this->instances[$driverName] = array();
+
+ return $this;
+ }
+
+ /**
+ * Get a certain instance from the cache manager
+ *
+ * @param string $driver
+ * @param int $instanceId
+ *
+ * @return bool
@stof
Symfony member
stof added a line comment Nov 3, 2012

really ?

@dlsniper
dlsniper added a line comment Nov 3, 2012

Mistake :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Nov 3, 2012
src/Symfony/Component/Cache/Cache.php
+
+ /**
+ * Get a certain instance from the cache manager
+ *
+ * @param string $driver
+ * @param int $instanceId
+ *
+ * @return bool
+ */
+ public function getDriverInstance($driver, $instanceId)
+ {
+ $driver = strtolower($driver);
+ $instanceId = $instanceId;
+
+ if (!isset($this->drivers[$driver]) || !(isset($this->instances[$driver][$instanceId]))) {
+ return false;
@stof
Symfony member
stof added a line comment Nov 3, 2012

This method is meant to return a driver instance. So you should throw an exception when it is not available, not returning false (btw, returning false seems weird. It should be null if you don't want to report errors, not a boolean)

@dlsniper
dlsniper added a line comment Nov 3, 2012

Thanks, that one was a todo for the next commit :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 3, 2012
src/Symfony/Component/Cache/Cache.php
+ }
+
+ /**
+ * Get a certain instance from the cache manager
+ *
+ * @param string $driver
+ * @param int $instanceId
+ *
+ * @return bool
+ */
+ public function getDriverInstance($driver, $instanceId)
+ {
+ $driver = strtolower($driver);
+ $instanceId = $instanceId;
+
+ if (!isset($this->drivers[$driver]) || !(isset($this->instances[$driver][$instanceId]))) {
@stof
Symfony member
stof added a line comment Nov 3, 2012

checking isset($this->instances[$driver][$instanceId]) is enough

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Nov 3, 2012
src/Symfony/Component/Cache/Cache.php
+ * @param string $driver
+ * @param array $configuration
+ * @param string $instanceName
+ *
+ * @return int|bool
+ */
+ public function spawnDriverInstance($driver, $configuration = array(), $instanceName = '')
+ {
+ $driver = strtolower($driver);
+
+ if (!isset($this->drivers[$driver])) {
+ return false;
+ }
+
+ if ($instanceName == '') {
+ $this->instances[$driver][] = new $this->drivers[$driver]($configuration);
@stof
Symfony member
stof added a line comment Nov 3, 2012

what about drivers with dependencies ? For instance, a cache implemented with a database and needing a DBAL connection ?

@dlsniper
dlsniper added a line comment Nov 3, 2012

Shouldn't that be either handled in the constructor, like for Memcached or sent as a configuration dependency?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 3, 2012
src/Symfony/Component/Cache/Drivers/Apc.php
+ *
+ * @author Florin Patan <florinpatan@gmail.com>
+ */
+class Apc implements CacheDriverInterface
+{
+
+ /**
+ * TTL
+ *
+ * @var int
+ */
+ private $ttl = 600;
+
+ public function __construct($config)
+ {
+ if (!is_array($config)) {
@stof
Symfony member
stof added a line comment Nov 3, 2012

why not just using a typehint ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 3, 2012
src/Symfony/Component/Cache/Drivers/Apc.php
+ {
+ return apc_fetch($key);
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function has($key)
+ {
+ return apc_exists($key);
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function set($id, $data, $lifeTime = -1)
@stof
Symfony member
stof added a line comment Nov 3, 2012

why using a different default value than the interface ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Nov 3, 2012
src/Symfony/Component/Cache/Drivers/Memcached.php
+ */
+ public function set($id, $data, $lifeTime = -1)
+ {
+ if ($lifeTime == -1) {
+ $lifeTime = $this->ttl;
+ }
+
+ return (bool)apc_store($id, $data, (int)$lifeTime);
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function remove($id)
+ {
+ return apc_delete($id);
@stof
Symfony member
stof added a line comment Nov 3, 2012

WTF ? This is a memcached driver

@dlsniper
dlsniper added a line comment Nov 3, 2012

Seems I've pushed an older version of this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof
Symfony member

@dlsniper The issue is that what you haven't taken into account is a discussion about the architecture of the component. So it means that taking it into account requires rewriting your component almost entirely.

@stof stof commented on an outdated diff Nov 3, 2012
src/Symfony/Component/Cache/Container/CacheContainer.php
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function useMetadata($useMetadata)
+ {
+ $this->useMetadata = $useMetadata;
+
+ return $this;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function setRawMetadata($metadata)
@stof
Symfony member
stof added a line comment Nov 3, 2012

you should typehint the argument as an array

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Nov 3, 2012
src/Symfony/Component/Cache/Container/CacheContainer.php
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function hasMetadata()
+ {
+ return count($this->metadata) > 0;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function hasMetadataKey($key)
+ {
+ return isset($this->metadata[$key]) || array_key_exists($key, $this->metadata);
@stof
Symfony member
stof added a line comment Nov 3, 2012

why using both isset and array_key_exists ?

@dlsniper
dlsniper added a line comment Nov 4, 2012

Extreme optimization, I'll remove it as most likely it won't be a problem given the amount of variables we expect to have in this array.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Nov 3, 2012
src/Symfony/Component/Cache/Container/CacheContainer.php
+ * {@inheritdoc}
+ */
+ public function hasMetadataKey($key)
+ {
+ return isset($this->metadata[$key]) || array_key_exists($key, $this->metadata);
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getMetadata()
+ {
+ return $this->metadata;
+ }
+
+ public function getMetadataKey($key)
@stof
Symfony member
stof added a line comment Nov 3, 2012

This does not return a key

@dlsniper
dlsniper added a line comment Nov 4, 2012

I'm not sure how to name it. If you have any ideas, let me know, the only alternative I got currently is getMetadataKeyValue() or getMetadataKeyInformation()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 3, 2012
src/Symfony/Component/Cache/Container/CacheContainer.php
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function setRawMetadata($metadata)
+ {
+ $this->metadata = $metadata;
+
+ return $this;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function setMetadataKey($key, $value)
@stof
Symfony member
stof added a line comment Nov 3, 2012

This does not set a key

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 3, 2012
src/Symfony/Component/Cache/Cache.php
+
+ /**
+ * Remove a certain instance from the current instances
+ *
+ * @param string $driver
+ * @param int|string $instanceId
+ *
+ * @return Cache
+ */
+ public function despawnDriverInstance($driver, $instanceId)
+ {
+ if (!$this->hasDriverInstance($driver, $instanceId)) {
+ return $this;
+ }
+
+ $driver = strtolower($driver);
@stof
Symfony member
stof added a line comment Nov 3, 2012

there is no doDespawn method

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dlsniper

Things left to be done that I'm currently not sure about:

  • implement namespaces if needed -> I'm a bit unsure how/why should we do this. It could be added as a metadata field and then used when the user saves the object into the cache. If it's ok like that, then implementation should be a matter of minutes;

  • implement locking mechanism for writing -> On this one I'm a bit against it as I can't find a use case for it. If the cache needs to be written by the user and it takes a lot of resources, then the application is not implemented correctly. If the cache needs to be populated by the users and it doesn't take much resources then who cares? If the application has a large traffic volume and cache is generated by the users then the problem is the application designer as for such application the cache should be user-pulled not user-pushed. So if you find any use cases that I didn't covered or I was wrong, do tell;

  • tagging: I'll implement this as described for namespaces. Add a special key to the metadata, called tags, then have a special key that can hold tags => keynames.

@stof stof commented on an outdated diff Nov 6, 2012
src/Symfony/Component/Cache/Cache.php
+ * Add a driver to our drivers list
+ *
+ * @param string $driverName Name of the driver to be added
+ * @param string $driverClass Class of the driver to be used
+ *
+ * @return Cache
+ *
+ * @throws \RuntimeException If the specified driver doesn't implement the CacheDriverInterface
+ */
+ public function addDriver($driverName, $driverClass)
+ {
+ if ($this->hasDriver($driverName)) {
+ return $this;
+ }
+
+ if ($this->debugMode && $this->logger) {
@stof
Symfony member
stof added a line comment Nov 6, 2012

why this check on the debug mode before logging at the debug level ? There is absolutely no such code in Symfony currently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 6, 2012
src/Symfony/Component/Cache/Cache.php
+ */
+ public function setLogger(LoggerInterface $logger)
+ {
+ $this->logger = $logger;
+
+ return $this;
+ }
+
+ /**
+ * Return the logger, if used
+ *
+ * @return null|LoggerInterface
+ */
+ public function getLogger()
+ {
+ return $this->logger;
@stof
Symfony member
stof added a line comment Nov 6, 2012

Why putting a getter for the logger ? This class is not meant to provide a way to access the logger for others. It does not make sense to have this getter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Nov 6, 2012
src/Symfony/Component/Cache/Cache.php
+ 'instance' => $driverInstance
+ );
+
+ // Is this a named instance?
+ if ($instanceId == '') {
+ $this->instances[] = $instance;
+ end($this->instances);
+
+ $result = key($this->instances);
+ } else {
+ $this->instances[$instanceId] = $instance;
+
+ $result = $instanceId;
+ }
+
+ return $result;
@stof
Symfony member
stof added a line comment Nov 6, 2012

missing typehint

@dlsniper
dlsniper added a line comment Nov 6, 2012

There's no need for typehinting there are the $result ca be either a string or a integer.

@stof
Symfony member
stof added a line comment Nov 6, 2012

arf, you edited the PR after I loaded the page, so it changed the line numbers between the JS in my browser and the backend. The comment was meant to be on the method declaration of addDriverInstances

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 6, 2012
src/Symfony/Component/Cache/CacheProxy.php
+ *
+ * @var bool
+ */
+ private $debugMode = false;
+
+ /**
+ * Cache profiler
+ *
+ * @var null|CacheProfiler
+ */
+ private $profiler = null;
+
+ /**
+ * Create our proxy so that we can use objects to our drivers and have other cool things like logging and profiling
+ *
+ * @param Drivers\CacheDriverInterface $driver
@stof
Symfony member
stof added a line comment Nov 6, 2012

This should be simply CacheDriverInterface as you have a use statement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 6, 2012
src/Symfony/Component/Cache/CacheProxy.php
+ }, $keys);
+ }
+
+ /**
+ * Make sure that the value passed is a object
+ *
+ * @param string $key
+ * @param mixed $value
+ *
+ * @return Container\CacheContainerInterface
+ */
+ private function ensureCacheContainer($key, $value)
+ {
+ switch (true) {
+ case $value instanceof CacheContainerInterface: return $value; break;
+ case !is_string($value): return new CacheContainer($key, $value); break;
@stof
Symfony member
stof added a line comment Nov 6, 2012

please follow the coding standards:

  • break is dead code so it should be removed
  • the return statement should be on its own line
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Nov 6, 2012
src/Symfony/Component/Cache/CacheProxy.php
+ }
+
+ /**
+ * Make sure that the value passed is a object
+ *
+ * @param string $key
+ * @param mixed $value
+ *
+ * @return Container\CacheContainerInterface
+ */
+ private function ensureCacheContainer($key, $value)
+ {
+ switch (true) {
+ case $value instanceof CacheContainerInterface: return $value; break;
+ case !is_string($value): return new CacheContainer($key, $value); break;
+ case false === $val = @unserialize($value): return new CacheContainer($key, $val); break;
@stof
Symfony member
stof added a line comment Nov 6, 2012

why this case ?

@dlsniper
dlsniper added a line comment Nov 6, 2012

say the unserialized value is an object that was stored by someone directly, in that case you'll at least be able to use the other methods before performing operations on the stored value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 6, 2012
...fony/Component/Cache/Drivers/CacheDriverInterface.php
@@ -0,0 +1,61 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+namespace Symfony\Component\Cache\Drivers;
@stof
Symfony member
stof added a line comment Nov 6, 2012

The namespace should be singular

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Nov 6, 2012
...nt/Cache/Drivers/MultipleCacheOperationsInterface.php
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+namespace Symfony\Component\Cache\Drivers;
+
+/**
+ * Interface for cache drivers that can support multiple operations at once
+ *
+ * See PSR proposal
+ * @link https://github.com/evert/fig-standards/blob/master/proposed/objectcache.md
+ *
+ * @author Florin Patan <florinpatan@gmail.com>
+ */
+interface MultipleCacheOperationsInterface extends CacheDriverInterface
@stof
Symfony member
stof added a line comment Nov 6, 2012

The name is really weird. A class implementing this interface does not represent multiple cache operations. It is a driver supporting batch operations

@dlsniper
dlsniper added a line comment Nov 6, 2012

Would BulkInterface be any better? Again, I'm really not that good at naming stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Nov 6, 2012
src/Symfony/Component/Cache/Drivers/Memcached.php
+
+ /**
+ * Memcached instance
+ *
+ * @var \Memcached
+ */
+ private $memcached;
+
+ /**
+ * Create our Memcached driver interface
+ *
+ * @param array $config
+ *
+ * @throws \RuntimeException When the configuration array doesn't provide all necesary information
+ */
+ public function __construct(array $config)
@stof
Symfony member
stof added a line comment Nov 6, 2012

I would pass a Memcached instance and then make the Cachebundle use the DIC to configure it.

@dlsniper
dlsniper added a line comment Nov 6, 2012

This is only when you don't use the CacheProxy, which I think should be named CacheDriverProxy.
That will take care of doing the instance if you don't provide one already. I'll update the bundle to reflect that soon.

@stof
Symfony member
stof added a line comment Nov 7, 2012

What are you talking about ? Your MemcachedDriver requires a config array to build the Memcached connection internally in all cases, instead of following the DI pattern

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Nov 6, 2012
...Component/Cache/Container/CacheContainerInterface.php
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+namespace Symfony\Component\Cache\Container;
+
+/**
+ * Interface for caching object
+ *
+ * @author Florin Patan <florinpatan@gmail.com>
+ */
+interface CacheContainerInterface
@stof
Symfony member
stof added a line comment Nov 6, 2012

I find the name weird. Using container for an object representing a single cached item looks wrong to me.

@dlsniper
dlsniper added a line comment Nov 6, 2012

Would CacheObject be better? I'm not very good at naming stuff.

@stof
Symfony member
stof added a line comment Nov 7, 2012

I would call it CacheItem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Nov 6, 2012
src/Symfony/Component/Cache/CacheProxy.php
+ if ($this->driver instanceof Drivers\MultipleCacheOperationsInterface) {
+ $result = $this->driver->existsMultiple($keys);
+ } else {
+ $result = array();
+ foreach ($keys as $key) {
+ $result[] = $this->driver->exists($key);
+ }
+ }
+
+ return $result;
+ }
+
+ /**
+ * Convert the values of an array to strings only
+ *
+ * @param string[]|Container\CacheContainerInterface[]|mixed[] $keys
@stof
Symfony member
stof added a line comment Nov 6, 2012

mixed[] means any array, so it looks weird to provide the others

@dlsniper
dlsniper added a line comment Nov 6, 2012

I'm not sure how to tell that it supports mixed values, either string or objects

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 6, 2012
src/Symfony/Component/Cache/CacheProxy.php
+ foreach ($keys as $key) {
+ $result[] = $this->driver->exists($key);
+ }
+ }
+
+ return $result;
+ }
+
+ /**
+ * Convert the values of an array to strings only
+ *
+ * @param string[]|Container\CacheContainerInterface[]|mixed[] $keys
+ *
+ * @return string[]
+ */
+ private function convertKeysToString($keys)
@stof
Symfony member
stof added a line comment Nov 6, 2012

you should typehint the array

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 6, 2012
src/Symfony/Component/Cache/CacheProxy.php
+ */
+namespace Symfony\Component\Cache;
+
+use Symfony\Component\Cache\Drivers\CacheDriverInterface;
+use Symfony\Component\Cache\Drivers\SerializationSupportInterface;
+use Symfony\Component\Cache\Drivers\MultipleCacheOperationsInterface;
+use Symfony\Component\Cache\Container\CacheContainerInterface;
+use Symfony\Component\HttpKernel\Log\LoggerInterface;
+use Symfony\Component\Cache\Container\CacheContainer;
+
+/**
+ * This is our cache proxy
+ *
+ * @author Florin Patan <florinpatan@gmail.com>
+ */
+class CacheProxy
@stof
Symfony member
stof added a line comment Nov 6, 2012

The naming is weird for a class which will be the frontend of the Cache component

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
...Component/Cache/Container/CacheContainerInterface.php
@@ -0,0 +1,127 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+namespace Symfony\Component\Cache\Container;
@stof
Symfony member
stof added a line comment Nov 7, 2012

missing empty line between the license header and the namespace declaration (everywhere)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof
Symfony member

and of course, you should add tests

@stof stof commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/Cache.php
+ * Proxy class that should be between user and the cache driver
+ * If none is set then the driver should be exposed directly
+ * but the user won't be able to use Container\CacheContainerInterface
+ * to store objects
+ *
+ * @var string
+ */
+ private $proxyClass = '';
+
+ /**
+ * Debug mode flag
+ * Takes care of logging and profiling
+ *
+ * @var bool
+ */
+ private $debugMode = false;
@stof
Symfony member
stof added a line comment Nov 7, 2012

you don't need this flag anymore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/Cache.php
+ }
+
+ $driverType = strtolower($driverType);
+
+ // Check that we don't have a already existing instance
+ if (isset($this->instances[$instanceId]['instance'])) {
+ $driverInstance = $this->instances[$instanceId]['instance'];
+ } else {
+ $driverInstance = new $this->drivers[$driverType]($configuration);
+ }
+
+ // If we have a proxy class the everything should be wrapped in that proxy
+ if ($this->proxyClass != '') {
+ $proxy = new $this->proxyClass($driverInstance, $this->debugMode);
+ $proxy
+ ->setLogger($this->logger)
@stof
Symfony member
stof added a line comment Nov 7, 2012

This is a FatalError if you don't have a logger as the typehint does not accept null (see, a test would be needed)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/CacheProxy.php
+ *
+ * @return CacheContainerInterface
+ */
+ private function ensureCacheContainer($key, $value)
+ {
+ switch (true) {
+ case $value instanceof CacheContainerInterface:
+ return $value;
+ case !is_string($value):
+ return new CacheContainer($key, $value);
+ // In case someone has decided to place another object directly into our cache
+ // we should deliver it to the application and let the user handle it
+ case false === $val = @unserialize($value):
+ return new CacheContainer($key, $val);
+ default:
+ return $value;
@stof
Symfony member
stof added a line comment Nov 7, 2012

Your default case is broken: it will not respect the contract of the method as it will not return a CacheContainerInterface

@stof
Symfony member
stof added a line comment Nov 7, 2012

btw, the only thing you can return here is a string

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/CacheProxy.php
+ * @param string $key
+ * @param mixed $value
+ *
+ * @return CacheContainerInterface
+ */
+ private function ensureCacheContainer($key, $value)
+ {
+ switch (true) {
+ case $value instanceof CacheContainerInterface:
+ return $value;
+ case !is_string($value):
+ return new CacheContainer($key, $value);
+ // In case someone has decided to place another object directly into our cache
+ // we should deliver it to the application and let the user handle it
+ case false === $val = @unserialize($value):
+ return new CacheContainer($key, $val);
@stof
Symfony member
stof added a line comment Nov 7, 2012

why doing a special case for a serialized false value ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/Container/CacheContainer.php
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getValue()
+ {
+ return $this->value;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function setTtl($ttl)
+ {
+ if ($ttl == null) {
@stof
Symfony member
stof added a line comment Nov 7, 2012

should be null === $ttl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/Container/CacheContainer.php
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function useMetadata($useMetadata)
+ {
+ $this->useMetadata = $useMetadata;
+
+ return $this;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function setRawMetadata(array $metadata)
@stof
Symfony member
stof added a line comment Nov 7, 2012

why raw ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/Container/CacheContainer.php
+ */
+ private $defaultTtl = 600;
+
+ /**
+ * Metadata information for the cached object
+ *
+ * @var array
+ */
+ private $metadata = array();
+
+ /**
+ * Use metadata information
+ *
+ * @var bool
+ */
+ private $useMetadata = true;
@stof
Symfony member
stof added a line comment Nov 7, 2012

What is the goal of this private property which is written but never accessed ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/Driver/Memcached.php
+ unset($config['servers']);
+
+ if (count($config)) {
+ // @TODO find a way to apply the extra settings like various options at build time else the user
+ // should use the getRawDriver in order to set the options before using it
+ }
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function get($key)
+ {
+ $result = $this->memcached->get($key);
+ if ($result === false) {
+ if ($this->memcached->getResultCode() === \Memcached::RES_NOTFOUND) {
@stof
Symfony member
stof added a line comment Nov 7, 2012

you should use &&

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/Driver/Memcached.php
+ $result = $this->memcached->get($key);
+ if ($result === false) {
+ if ($this->memcached->getResultCode() === \Memcached::RES_NOTFOUND) {
+ $result = null;
+ }
+ }
+
+ return $result;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function exists($key)
+ {
+ return $this->memcached->get($key) ? true : $this->memcached->getResultCode() != \Memcached::RES_NOTFOUND;
@stof
Symfony member
stof added a line comment Nov 7, 2012

the comparison should be strict

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/Driver/Memcached.php
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function exists($key)
+ {
+ return $this->memcached->get($key) ? true : $this->memcached->getResultCode() != \Memcached::RES_NOTFOUND;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function set($key, $data, $lifeTime = null)
+ {
+ if ($lifeTime == null) {
@stof
Symfony member
stof added a line comment Nov 7, 2012

null === $lifetime

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/Driver/Memcached.php
+ */
+ public function getMultiple($keys)
+ {
+ return $this->memcached->getMulti($keys);
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function deleteMultiple($keys)
+ {
+ $count = count($keys);
+
+ for($i=0; $i<$count; $i++) {
+ $this->delete($keys[$i]);
+ }
@stof
Symfony member
stof added a line comment Nov 7, 2012
foreach ($keys as $key) {
    $this->delete($key);
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/Driver/Memcached.php
+ $count = count($keys);
+
+ for($i=0; $i<$count; $i++) {
+ $this->delete($keys[$i]);
+ }
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function existsMultiple($keys)
+ {
+ $result = array();
+
+ $count = count($keys);
+ for($i=0; $i<$count; $i++) {
@stof
Symfony member
stof added a line comment Nov 7, 2012

same here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
...ent/Cache/Driver/MultipleCacheOperationsInterface.php
+ * @param int $ttl
+ *
+ * @return void
+ */
+ public function setMultiple(array $items, $ttl = null);
+
+ /**
+ * Fetches multiple items from the cache.
+ *
+ * The returned structure must be an associative array. If items were
+ * not found in the cache, they should not be included in the array.
+ *
+ * This means that if none of the items are found, this method must
+ * return an empty array.
+ *
+ * @param string $keys
@stof
Symfony member
stof added a line comment Nov 7, 2012

string ? seriously ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
...ent/Cache/Driver/MultipleCacheOperationsInterface.php
+ * return an empty array.
+ *
+ * @param string $keys
+ *
+ * @return array
+ */
+ public function getMultiple($keys);
+
+ /**
+ * Deletes multiple items from the cache at once.
+ *
+ * @param array $keys
+ *
+ * @return void
+ */
+ public function deleteMultiple($keys);
@stof
Symfony member
stof added a line comment Nov 7, 2012

the array should be typehinted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof
Symfony member

Renaming the file to src/Symfony/Component/Cache/{ → Item}/Container/CacheContainerInterface.php without doing any change in it means that either it was broken previously or it is broken now.
Seriously, write unit tests

@stof
Symfony member

Thus, the new filename does not correspond to the way you renamed the use statements in other classes either => 3 diffferent naming for the same class

@stof stof and 1 other commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/Cache.php
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+namespace Symfony\Component\Cache;
+
+use Symfony\Component\Cache\Driver;
+use Symfony\Component\Cache\Driver\CacheDriverInterface;
+use Symfony\Component\HttpKernel\Log\LoggerInterface;
+
+/**
+ * This is our cache manager
+ *
+ * @author Florin Patan <florinpatan@gmail.com>
+ */
+class Cache
@stof
Symfony member
stof added a line comment Nov 7, 2012

This class is both a factory and a registry (so breaking the Single Responsibility Principle) and does not follow the DI pattern (as much of your Pr actually).

If you want a CacheRegistry, it should receive configured drivers IMO, not a class name to instantiate it. And then, simply create a LazyCacheRegistry lazy-loading them from the DIC for people using the DI component, to improve the performances by instantiating the drivers only when needed.
And btw, if you want a registry, the class name should reflect it.

@dlsniper
dlsniper added a line comment Nov 7, 2012

I'm really not sure about the scope right now, I'll think of it next days.
I added the whole lazy loading part because I was thinking about the uses cases where one shouldn't instantiate any cache driver before actually using it and also for the case when you don't use a DI component, like people who just want to use this in their code.

On the other hand, I don't know how to use the DI to create services on the fly, like those needed for the various caching drivers, so if you have any tutorials/links it would be much appreciated.

@stof
Symfony member
stof added a line comment Nov 7, 2012

The DI container only instantiate a service the first time you request it, which is why I said you should have a special LazyCacheRegistry with a reference to the container to lazy-load the services.
For a first implementation, I think the simple CacheRegistry (requiring to initialize all instances but without any other dependencies) and the LazyCacheRegistry are enough. People wanting another implementation (based on Pimple for instance) can also implement the interface.

@dlsniper
dlsniper added a line comment Nov 8, 2012

@stof I'm really clueless about this.

What I'm trying to do is something like this:

  • declare the cache drivers in the container
  • declare a service builder, like what Cache.php does right now, which should take the given configuration for a driver and create it;
  • make sure I don't get this twice in a row;
  • keep the same functionality for people not using the DI component.

While I can split the Cache.php file into a CachePool.php, to act as a registry, and CacheFactory to create the instantiated drivers, I'm not really sure how this can be done for the DI component, more accurate, I'm not sure how to register all the needed cache services so that the you can call them from via the DI component. Did I skipped something from the documentation?

Thanks!

@stof
Symfony member
stof added a line comment Nov 8, 2012

@dlsniper you should declare the instance in the container instead of declaring a config array. The instantiation of the driver would be done by the container.

People not using the DIC would build the instance themselves to add it in the registry (which is far more flexible than trying to build a big array and handling keys in many different ways to configure the dependency)

@dlsniper
dlsniper added a line comment Nov 9, 2012

I've been thinking a bit about this and I've reached the conclusion that it doesn't really make sense if people are not using a container to implement a registry for them. If by any chance they'll want lazy loading of the driver instances, the ones via CacheDriver.php then they would just need to load the driver into the CacheDriver then. And if they are using a DI other that Symfony2 one then they don't need one.

Also having a CacheFactory doesn't really provide any other improvements as either the DI or the use will handle it.

I'll drop this functionality from the PR and move it to the DI for the CacheBundle.

Thanks for enlightenment @stof :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
...mfony/Component/Cache/Driver/CacheDriverInterface.php
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+namespace Symfony\Component\Cache\Driver;
+
+/**
+ * Interface for cache drivers
+ *
+ * See PSR proposal
+ * @link https://github.com/evert/fig-standards/blob/master/proposed/objectcache.md
+ *
+ * @author Florin Patan <florinpatan@gmail.com>
+ */
+interface CacheDriverInterface
@stof
Symfony member
stof added a line comment Nov 7, 2012

I would rename it to DriverInterface. Cache is already in the namespace of the component

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
...ent/Cache/Driver/MultipleCacheOperationsInterface.php
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+namespace Symfony\Component\Cache\Driver;
+
+/**
+ * Interface for cache drivers that can support multiple operations at once
+ *
+ * See PSR proposal
+ * @link https://github.com/evert/fig-standards/blob/master/proposed/objectcache.md
+ *
+ * @author Florin Patan <florinpatan@gmail.com>
+ */
+interface MultipleCacheOperationsInterface extends CacheDriverInterface
@stof
Symfony member
stof added a line comment Nov 7, 2012

I would suggest BatchDriverInterface

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof
Symfony member

@dlsniper I would be easier to avoid squashing commits during the work. Seeing the different commits allow me to see the changes since my previous review more easily. Squashing should be done at the end when the implementation is ready.

@stof stof commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/Cache.php
+
+ if ($this->logger) {
+ $this->logger->debug(sprintf('Spawned driver instance %s with driver %s', $instanceId, $driverType));
+ }
+
+ return true;
+ }
+
+ /**
+ * Remove a certain instance from the current instances
+ *
+ * @param int|string $instanceId
+ *
+ * @return Cache
+ */
+ public function removeDriverInstance($instanceId)
@stof
Symfony member
stof added a line comment Nov 7, 2012

public methods should be defined before private ones

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/CacheProxy.php
+ * Cache profiler
+ *
+ * @var null|CacheProfiler
+ */
+ private $profiler = null;
+
+ /**
+ * Create our proxy so that we can use objects to our drivers and have other cool things like logging and profiling
+ *
+ * @param DriverInterface $driver
+ * @param bool $debugMode
+ */
+ public function __construct(DriverInterface $driver, $debugMode = false)
+ {
+ $this->driver = $driver;
+ $this->debugMode = $debugMode;
@stof
Symfony member
stof added a line comment Nov 7, 2012

This flag is unused (and the property is not declared anymore). You should remove it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/CacheProxy.php
+ */
+ public function getProfiler()
+ {
+ return $this->profiler;
+ }
+
+ /**
+ * Get cache entry
+ *
+ * @param string|CacheItemInterface $key
+ *
+ * @return mixed
+ */
+ public function get($key)
+ {
+ $CacheItem = $this->ensureCacheItem($key, 0);
@stof
Symfony member
stof added a line comment Nov 7, 2012

the first letter of the variable name should be lowercased

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dlsniper

@stof thanks, I won't squash it anymore, I've fixed most of the things pointed out and since their were quite a bit of commits I thought it would be easier to have a cleaner base but I guess I went a bit too far. Sorry.

@stof stof and 1 other commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/CacheProxy.php
+ }
+
+ /**
+ * Check if multiple keys exists in the cache
+ *
+ * @param string[]|Item\CacheItemInterface[]|mixed[] $keys
+ *
+ * @return bool[]
+ */
+ public function existsMultiple($keys)
+ {
+ $keys = $this->convertKeysToString($keys);
+
+ if ($this->logger) {
+ $this->logger->debug('Checking cache for ' . implode(', ', $keys));
+ }
@stof
Symfony member
stof added a line comment Nov 7, 2012

do you really need to log each call to the cache ? It looks like it will be spamming logs

@dlsniper
dlsniper added a line comment Nov 7, 2012

To be honest I'm not sure if we need logging on that level as well, that's why I've had the $debugMode variable but even so, I could remove this logging for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/CacheProxy.php
+ *
+ * @return CacheItemInterface
+ */
+ private function ensureCacheItem($key, $value)
+ {
+ switch (true) {
+ case $value instanceof CacheItemInterface:
+ return $value;
+ case !is_string($value):
+ return new CacheItem($key, $value);
+ // In case someone has decided to place another object directly into our cache
+ // we should deliver it to the application and let the user handle it
+ case false === $val = @unserialize($value):
+ return new CacheItem($key, $val);
+ default:
+ return $value;
@stof
Symfony member
stof added a line comment Nov 7, 2012

the default case does not respect the contract of the method as it does not return a CacheItemInterface (which will break the code btw as the other methods of the class expect the contract to be respected)

@dlsniper
dlsniper added a line comment Nov 7, 2012

Yes, you are right, I'll make the default to just return the new CacheItem object. Thanks

@dlsniper
dlsniper added a line comment Nov 7, 2012

This will certainly have unit tests done, but I first need to get the component into a better shape and have the time for writing the tests. If anyone is willing to help...

@stof
Symfony member
stof added a line comment Nov 7, 2012

Well, writing unit tests at the same time than the code would ensure that your code works. In the current PR, there is several places triggering fatal errors or notices

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/Driver/Apc.php
@@ -0,0 +1,119 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+namespace Symfony\Component\Cache\Driver;
@stof
Symfony member
stof added a line comment Nov 7, 2012

missing empty line between the license header and the namespace declaration (in all files)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/Driver/Apc.php
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function deleteMultiple($keys)
+ {
+ apc_delete($keys);
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function existsMultiple($keys)
+ {
+ return apc_exists($keys);
@stof
Symfony member
stof added a line comment Nov 7, 2012

This does not respect the interface: inexistant keys will be missing in the returned array instead of having a value set to false

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Nov 7, 2012
...mfony/Component/Cache/Driver/BatchDriverInterface.php
+ public function setMultiple(array $items, $ttl = null);
+
+ /**
+ * Fetches multiple items from the cache.
+ *
+ * The returned structure must be an associative array. If items were
+ * not found in the cache, they should not be included in the array.
+ *
+ * This means that if none of the items are found, this method must
+ * return an empty array.
+ *
+ * @param string[] $keys
+ *
+ * @return array
+ */
+ public function getMultiple($keys);
@stof
Symfony member
stof added a line comment Nov 7, 2012

you should typehint the argument

@dlsniper
dlsniper added a line comment Nov 7, 2012

The interface is taken from PSR proposal and I've tried to keep accurate to it. Should I still make this change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
...mfony/Component/Cache/Driver/BatchDriverInterface.php
+ * return an empty array.
+ *
+ * @param string[] $keys
+ *
+ * @return array
+ */
+ public function getMultiple($keys);
+
+ /**
+ * Deletes multiple items from the cache at once.
+ *
+ * @param array $keys
+ *
+ * @return void
+ */
+ public function deleteMultiple($keys);
@stof
Symfony member
stof added a line comment Nov 7, 2012

same here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
...mfony/Component/Cache/Driver/BatchDriverInterface.php
+ *
+ * @return void
+ */
+ public function deleteMultiple($keys);
+
+ /**
+ * Check for multiple items if they appear in the cache.
+ *
+ * All items must be returned as an array. And each must array value
+ * must either be set to true, or false.
+ *
+ * @param array $keys
+ *
+ * @return array
+ */
+ public function existsMultiple($keys);
@stof
Symfony member
stof added a line comment Nov 7, 2012

and here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/Driver/DriverInterface.php
+
+ /**
+ * Check if an entry exists in cache
+ *
+ * @param string $key Entry id
+ *
+ * @return boolean
+ */
+ public function exists($key);
+
+ /**
+ * Get an entry from the cache
+ *
+ * @param string $key Entry id
+ *
+ * @return mixed The cached data or FALSE
@stof
Symfony member
stof added a line comment Nov 7, 2012

and how would I distinguish a cache miss from a cached false value ?

@dlsniper
dlsniper added a line comment Nov 7, 2012

The exists function should help you there. But who would store false in cache anyway? And even so, if you are using Cache\CacheProxy then it shouldn't be a problem to retrieve false from the cache.

@dlsniper
dlsniper added a line comment Nov 7, 2012

Also, this is because of the PSR proposal attempt, or whatever that is, from one of those two earlier tickets.
There's still no direction to actually know what interface should actually be implemented so I've just implemented that and I hope to get some feedback on it.

@stof
Symfony member
stof added a line comment Nov 7, 2012

Well, what if I have an heavy calculation returning a boolean ? I may want to cache the result. The Cache component should not make any assumption about what is stored (well, except required assumptions like the fact that object cached need to be serializable).

And indeed, your CacheProxy solves the issue when using it. But you seem to consider it like an optional extra feature on top of the driver whereas the discussion was suggesting to separate totally the cache frontend (used by userland code) and the cache backend (your drivers here).
If we keep the driver as is for the backend, I would personally vote for using null instead of false (which is what you do in the implementations already)

@dlsniper
dlsniper added a line comment Nov 8, 2012

Since the benefits of having a object serialized into cache far out-weights the cons, imho at least, I've made the CacheProxy mandatory, so that you can store anything you want/need in the cache systems without fear that you can't store a certain data-type.

Also, I'm thinking about using the Serializer Component, to my shame I've never used it so far, do you think it would make sense to use it here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/Driver/Apc.php
+ {
+ if (isset($config['ttl'])) {
+ $this->ttl = $config['ttl'];
+ }
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function get($key)
+ {
+ $found = true;
+ $result = apc_fetch($key, $found);
+
+ if (!$found) {
+ $result = null;
@stof
Symfony member
stof added a line comment Nov 7, 2012

this does not respect the interface as you wrote it should return false in this case (I don't agree with the choice of the interface but this is a different discussion)

@dlsniper
dlsniper added a line comment Nov 7, 2012

Yes, that's the problem, I didn't knew which interface to take and add to this PR.
The PSR one seems dead and a bit different from the pov of the people in the above tickets so.. depending on which direction we take, I'll change this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/Driver/Memcached.php
+
+ /**
+ * Memcached instance
+ *
+ * @var \Memcached
+ */
+ private $memcached;
+
+ /**
+ * Create our Memcached driver interface
+ *
+ * @param array $config
+ *
+ * @throws \RuntimeException When the configuration array doesn't provide all necesary information
+ */
+ public function __construct(array $config)
@stof
Symfony member
stof added a line comment Nov 7, 2012

As said previously (but my comment disappeared because of the renaming of the file), this driver should follow the DI pattern instead of instantiating the dependencies itself (write the unit tests and you will see it will help you)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/Driver/Memcached.php
+ unset($config['servers']);
+
+ if (count($config)) {
+ // @TODO find a way to apply the extra settings like various options at build time else the user
+ // should use the getRawDriver in order to set the options before using it
+ }
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function get($key)
+ {
+ $result = $this->memcached->get($key);
+ if (false === $result && \Memcached::RES_NOTFOUND === $this->memcached->getResultCode()) {
+ $result = null;
@stof
Symfony member
stof added a line comment Nov 7, 2012

this does not respect the interface either

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/Driver/Memcached.php
+ public function deleteMultiple($keys)
+ {
+ foreach ($keys as $key) {
+ $this->delete($keys[$key]);
+ }
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function existsMultiple($keys)
+ {
+ $result = array();
+
+ foreach ($keys as $key) {
+ $result[$keys[$key]] = $this->exists($keys[$key]);
@stof
Symfony member
stof added a line comment Nov 7, 2012

$keys[$key] is totally wrong. $key is already the value in the $keys array.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/Item/CacheItem.php
+ */
+ private $key = '';
+
+ /**
+ * Cache value
+ *
+ * @var mixed
+ */
+ private $value = '';
+
+ /**
+ * TTL of the object in cache
+ *
+ * @var int
+ */
+ private $ttl = 600;
@stof
Symfony member
stof added a line comment Nov 7, 2012

why putting a default value here ? The constructor always erases it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/Item/CacheItem.php
+class CacheItem implements CacheItemInterface
+{
+
+ /**
+ * Cache key
+ *
+ * @var string
+ */
+ private $key = '';
+
+ /**
+ * Cache value
+ *
+ * @var mixed
+ */
+ private $value = '';
@stof
Symfony member
stof added a line comment Nov 7, 2012

Why initializing them to empty strings ? It is useless as the constructor always overwrites them

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/Item/CacheItem.php
+ */
+ private $defaultTtl = 600;
+
+ /**
+ * Metadata information for the cached object
+ *
+ * @var array
+ */
+ private $metadata = array();
+
+ /**
+ * Use metadata information
+ *
+ * @var bool
+ */
+ private $useMetadata = true;
@stof
Symfony member
stof added a line comment Nov 7, 2012

This flag is written but never accessed, meaning it is useless

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/Item/CacheItem.php
+ /**
+ * {@inheritdoc}
+ */
+ public function setMetadataParameter($key, $value)
+ {
+ $this->metadata[$key] = $value;
+
+ return $this;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function hasMetadata()
+ {
+ return count($this->metadata) > 0;
@stof
Symfony member
stof added a line comment Nov 7, 2012

I would use !empty() here as we don't care about the count.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/Item/CacheItem.php
+ * @var mixed
+ */
+ private $value = '';
+
+ /**
+ * TTL of the object in cache
+ *
+ * @var int
+ */
+ private $ttl = 600;
+
+ /**
+ * This is default TTL, in case ttl is not specified
+ * @var int
+ */
+ private $defaultTtl = 600;
@stof
Symfony member
stof added a line comment Nov 7, 2012

The CacheItem represents a single item. So it seems weird to have a default ttl in it. An item has only its own ttl.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Nov 7, 2012
src/Symfony/Component/Cache/Item/CacheItemInterface.php
+ * Tell the object to store metadata or not
+ *
+ * @param bool $useMetadata
+ *
+ * @return CacheItemInterface
+ */
+ public function useMetadata($useMetadata);
+
+ /**
+ * Set the cache metadata
+ *
+ * @param array $metadata
+ *
+ * @return CacheItemInterface
+ */
+ public function setRawMetadata(array $metadata);
@stof
Symfony member
stof added a line comment Nov 7, 2012

why RawMetadata here whereas Metadata is used for the getter ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@asm89

What does this component add over integrating doctrine/common? (Or Stash?)

While I somewhat understand the "need" of a CacheInterface that can be used in the Symfony components without adding 3rd party dependencies, I do not see why Symfony should implement all the caching drivers by itself. If a general interface is added the component could have adapters to libraries like doctrine/common and Stash. Note that symfony/frameworkbundle already has a dependency on doctrine/common.

An earlier PR by @beberlei only implemented the ApcCache to implement the ApcUniversalClassLoader with it, but the PR can easily be extended to encourage the adapter approach: #3211. Of course there is also discussion on the original issue: #1513.

Let me ping @tedivm too. :)

@lsmith77

I think step one is to get a PSR for caching. For this it might be useful to really collect the requirements that Symfony2 has. From what I have seen we don't require a very fancy API at all. Once such an interface has been defined, we should look if we feel like we have something to add to the community with our own component. So yes we should definitely examine Doctrine Cache and Stash etc before adding another component to Symfony2.

@dlsniper

As @lsmith77 says, I want to gather input and actually do it, not just talk on it, like it happened on the mentioned PRs so that we could end up with something usable in 2.2, that's the main goal.

As for the other two points, depending on DoctrineCommon is not an option, it came out from the previous talks, we should have something of our own in Symfony and just have a bridge to use DoctrineCommon with it. As for Stash, I've looked into it but I believe things could be done in a much simpler way.

Like I've said, maybe there's no need to have a cache locking mechanism as most of the cache drivers to date have atomic operations and if you want your high traffic site to have the users populate the cache maybe your design is wrong not the cache component. This doesn't mean that if you want to have a transactional cache driver, you can't pass that to the config, that's what the config is used for.

Also, while I see a need for namespaces, that's something one could implement in a few lines of code once the component itself is stable enough to add extra features.

Same goes for unit tests and so on. I'd rather settle on something that most of the people agree with then add features rather that have a PR that covers adding support for this in other components, see the TODO list.

@stof
Symfony member

@dlsniper In the previous discussion, people were telling they want a different thing than Doctrine Common (which btw could be Stash). If the new cache component is doing exactly the same than Doctrine common, duplicating things is useless IMO.

@asm89

The component would suggest Stash or doctrine/common for the adapters. That's is probably no problem at all because in practice the frameworkbundle already has a dependency on doctrine/common and as soon as you start using anything with annotations, dbal or one odm/orm etc you will also have doctrine/common already.

+1 on your efforts to move this stuff forward btw :) I didn't state that before I see :)

@dlsniper

@asm89 thanks, I'm trying to get this done as I faced a similar need at my previous employer and it's not fun to say that the framework doesn't have any cache support built-in plus there are adapters flying all over the place. Since 2.2 is the last chance to add this before a LTS gets pushed.

@stof I don't plan to have a rewritten Doctrine/Common/Cahe component that's named Symfony/Component/Cache and maybe I opened this too early but I think that having early feedback would help me finish this in time and in a way that everyone would be happy with.

I know my coding skills might not be aligned to what Symfony2 expects, see the great job that @stof does at helping on this, but I'll gladly do anything that needs to be done in order to have this finished.

Also, as far as I remember, I'll need to revisit the original threads again, there was something that made people say that we shouldn't rely on doctrine/common for caching, but I've forgot the exact view on this. I know that we already have a dependency in FrameworkBundle for this but there could be other places where a standalone component might be better suited that having doctrine/common as a dependency.

If on the other hand, Doctrine guys would split the common lib into smaller libs then I don't think it would be a problem, but I'm not sure about this approach, maybe @beberlei could shed some light on this?

As for having a bridge to other solutions, well why not? That's the whole idea, I want to make something as pluggable as possible so that if you already use doctrine/common for caching, I think everyone does this for their DB layer, you could just tell the component to use that driver for the operations.

Also I'm open to any other suggestions that would actually move this forward so feel free to add as many ideas/comments as possible.

Thanks!

P.S.
I'm not sure about the experience of other people but I'm at my second work place where I'm trying to present Symfony2 as possible alternative to the aged Zend Framework and since most other companies I've heard of are extremely reluctant to having things changed, even if it means progress, having full option framework makes things a lot more easy do digest. Also it seems, at least in my area, that people are not just ready to embrace the new wave of frameworks/thinking about software components or software design and having even more new, separated from framework, things, unsettles people a bit. But maybe it's just because of enterprises/my area, I'm not sure about this.

@dlsniper dlsniper commented on the diff Nov 8, 2012
src/Symfony/Component/Cache/Item/CacheItem.php
+ /**
+ * {@inheritdoc}
+ */
+ public function setTtl($ttl)
+ {
+ $this->ttl = $ttl;
+
+ return $this;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getTtl()
+ {
+ return $this->ttl;
@dlsniper
dlsniper added a line comment Nov 8, 2012

I'm not sure about this, how should we treat the TTL for the users?
While setting it in seconds is a good thing, storing/getting it in the form of remaining TTL is another matter.
Should we have a different method called remainingTtl() which should provide the number of seconds until the object will expire?
For storing the TTL I think it should be done in form of having the a key named __tos (Time Of Save) in the metadata which should contain the UNIX timestamp for when the object was serialized.

Does the above make sense to anyone?

@dlsniper
dlsniper added a line comment Nov 14, 2012

I've added a method which acts like in the above description

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tedivm

@dlsniper have you looked at Stash and it's Symfony Bundle? A lot of what you're looking to do is already accomplished there, and there is a decent amount of active development going on.

@schmittjoh

I'd also encourage to take a look at that.

If there is a decent existing library, please let's not come up with something new. There is really no point.

@dlsniper

@tedivm I've just quickly browsed thru Stash but I haven't had time to have a good look on it, it's planed for this weekend.

Also I've started working on this as there was really nothing concrete done and from the existing attempts people either didn't wanted to use Doctrine/Common for drivers, as it would have introduced a bigger dependency or whatnot, or did want to have something in the code base and so on.

There's a couple of other things to consider as well.

At some point, someone suggested using Zend/Cache but then people started saying that it isn't that good as it depends on a couple of other Zend components which might also depend on other components and so on... Do you see the pattern here? Zend has its own solution but its bashed because its coupled with some other Zend components. Symfony2, if it adopts a third party library, is/isn't bashed for depending on third party library.

And keep in mind that if we'd have a Cache component, the rest of the components would need to implement it as they would have various benefits, see routing, annotations, universal loader, twig and whatever other components/bundles would need caching and this would introduce a dependency of the core components of Symfony to a very important library.

Please don't get me wrong, I'm not against using Stash and making a bridge to Symfony2 but I also don't see any reason why we shouldn't have something like this in the framework itself.

Maybe @fabpot should take a decision about this and I'd be more that happy to implement it either way, as long as things will get done and we have a working thing for 2.2 that covers the rest of the framework as well, see above.

So, as soon as @fabpot will take a decision, keep this going or switch to Stash/ZendCache/SomeOtherCacheLib I'll start doing the required changes but until then I'll keep reinventing the wheel (tm) as I'm aware that it's not an easy thing to decide.

Thank you all for your input, keep it coming!

@mvrhov

@dlsniper: Sorry to disappoint you, but I'm seeing this like a typical NIH syndrome. At least I consider stash just like any other 3rd party library sf already uses e.g. monolog or assetic
P.S. Also it's dependency tree is small just like the on from monolog or assetic

@fabpot
Symfony member

I had a quick look at Stash last week.

The first "issue" (which is very easy to fix) is documentation: there is not a single working example. Everything starts with a $pool var which is never created anywhere (or I've not found the proper/uptodate documentation which is also quite possible). Anyway, if you want to play with it, here is a small snippet of code:

<?php

require_once __DIR__.'/autoload.php';

$handler = new Stash\Handler\FileSystem();

$pool = new Stash\Pool();
$pool->setHandler($handler);

$stash = $pool->getCache('fruit');
$stash->set('apple');

var_dump($stash->get());
// string(6) "apples"

Apart from that, the way things are done sometimes looks a bit "weird" to me, but again, nothing that cannot be fixed.

I'm going to spend more time on Stash and give a more detailed feedback to @tedivm.

@dlsniper

@mvrhov maybe I wasn't clear when I actually said that I don't mind using anything that @fabpot decides, but until then I'll continue with one possible implementation for it.

Anyone that wants this to actually move forward should either help with useful comments, like @stof does, either make their own version which uses X library for it and submit a PR if they aren't happy with this.

Talking about doing stuff without actually doing them is worst that reinventing the wheel imho.

@dlsniper

@fabpot should I divert the efforts into making a bridge in Symfony to use Stash then?

@fabpot
Symfony member

@dlsniper We are not in a hurry (not yet at least). So, let's first review Stash before starting yet another PR. We already have 4 PRs for the cache, which is already more than enough.

If we decide that Stash is indeed the way to go, then, we will figure out how to integrate it into Symfony.

@tedivm

@fabpot There are two PR's right now on the Stash and Stash Documentation projects which may resolve some of the weirdness you mention. I'm pushing towards the next release, which does include a bit of API cleanup. The new documentation also contains more examples that include setting up the Pool class, and is being kept up to date with the API Cleanup changes.

API Cleanup
Documentation Rebuild

@dlsniper

@mvrhov I've looked into into Stash but I find it too bloated for a simple thing. Caching should just work and that's it. There's nothing bad in having something that doesn't implement 131241 patterns just to make everyone happy.

Also, after some serious talk with some friends, @c-datculescu and others, we've still haven't been able to find a proper use case for having a lock() method for cache. Just by adding it, it brings a lot of troubles to userland and like I've already stated above, if you find yourself in need to a lock on the cache system maybe you shouldn't do that in the first place.

I'll finish up with implementing namespaces then follow-up with tags and I think that's about it.

More drivers will then follow.

Please, keep an open mind about having something that looks simple, it seems that the article on Symfony.com which was stating that simplicity is a lost art, I believe that this was the one: http://blog.peterfisher.me.uk/2012/08/02/the-lost-art-of-simplicity-a-talk-by-josh-holmes/ I'll try and find the right one later on.

@stloyd stloyd and 1 other commented on an outdated diff Nov 12, 2012
src/Symfony/Component/Cache/.gitattributes
@@ -0,0 +1,2 @@
+Tests/ export-ignore
@stloyd
stloyd added a line comment Nov 12, 2012

Should be /Tests to work on Windows too.

@dlsniper
dlsniper added a line comment Nov 12, 2012

This is how currently all the other components have it. Should I make this change and also submit a PR for the rest of them?

@stloyd
stloyd added a line comment Nov 13, 2012

It was fixed recently in 2.1 but seems like not yet merged into master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stloyd stloyd commented on an outdated diff Nov 12, 2012
src/Symfony/Component/Cache/CacheDriver.php
+ } else {
+ return (string) $item;
+ }
+ }
+
+ /**
+ * Make sure that the value passed is a object
+ *
+ * @param string $key
+ * @param mixed $value
+ *
+ * @return CacheItemInterface
+ */
+ private function ensureCacheItem($key, $value)
+ {
+ switch (true) {
@stloyd
stloyd added a line comment Nov 12, 2012

Simple ifs would be a bit more readable IMO =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fabpot
Symfony member

@dlsniper Keep in mind that at this stage, we need to keep an open mind. We need to evaluate different options. If you think that Stash is not a good option, please share your concerns (I have some concerns as well that I will share once the Stash refactoring PR lands as it definitely goes in the right direction). But just saying that Stash is bloated does not help.

@dlsniper

@fabpot yes, I'll come with some concrete things about it but I'll wait for @tedivm to merge his PR first as maybe some things will either be more clear or improve as I didn't had the patience to check the diff as well.

Again, please take my opinion with a grain of salt, I might be biased but if we decide about anything, I'll be the first one either to help out either to make the PR.

Best regards.

@c-datculescu

Hello.

Since @dlsniper mentioned me i think i should make some of my thoughts more clear regarding locking.
First of all, enabling locking onto a mechanism that is supposed to be very fast is obviously enough a bad ideea. I cannot come up with a real reason for doing that.
That being said, comes the next problem: implementing a high level lock (as opposed to a low-level lock), aka. implementing a pseudo-locking mechanism in php is a terrifyingly bad idea. Let me rephrase: it cannot be done. If the caching server does not support locking mechanisms natively, then it has to mean something. Let's remember again that apc/memcache and the rest of servers used for caching are made for very fast concurrent access, which locking disables in various ways.
Also, locking mechanisms i can imagine for the caches are pessimistic by default. First get the lock, then do your job and release the lock. Which is actually pretty bad. Locks should be granted as further in time as possible, and should be released as fast as possible. Which is probably not the case here.

Let's analyze a little bit a situation. You have a query which result set you want to be cached. According to the locking schema described, you obtain the lock, holding all rest of clients in wait. You start doing your operations, and luckily you manage to finish. But what happens if the php process exits for example? you end up with a lock set in the database for some time. Let's say you set the lock for a small amount of time though. Now the problem is that the lock can expire before the operation finished, so other clients will think that the record is there. And they couldn't be more wrong.
In both cases, for a lock set to low time you get problems, for locks set with high times you also get big problems. Taking into account that there is no such thing as transaction enabled operations on either memcached/apc, you can also end up in problems regarding partially saved data if it happens to write multiple keys in a series.

If we also factor into this equation multiple server setup and so on, i think the conclusion about locking should be pretty obvious. In my opinion it is impossible to do without hitting problems later down the road, and these problems can be very hard to debug/manage.

PS: i have also seen mentions about fallback for a chosen caching mechanism. That can end up in circular reference which would be really really bad to get out of.
PS2: If you really want to implement some sort of locking, i think the only solution is to execute the code that needs to be cached on all clients that cannot acquire a lock, and let them continue with execution. As soon as the key becomes available, then it will be used by all successive calls.

Best regards,
Cristian.

@tedivm

@fabpot If there's anything you think I should add in as part of the refactoring job let me know. Development on that is going to slow down a little bit now that I'm out of the weekend (doing the real job during the week, with coding at night). There are a few more things planned, but I'd love any suggestions you have on top of that.

@dlsniper As I've been doing the refactoring I've also been updating the documentation. You should look at the dev site and see if that helps.

I am going to disagree with you about bloat. While it does have a lot of features, they are geared towards performance. Just because something has a lot of code doesn't mean it's bloated- in fact many of the items make it faster or remove various types of issues. For example, by building in some intelligent serialization into the Filesystem driver Stash actually manages to avoid having to unserialize most datatypes, resulting in much faster reads even though there's more code. As another example, when MetArt switched to Stash from their own native memcached calls they were actually able to remove some of their search servers do to the performance gains (even though they weren't caching anything new).

@c-datculescu Your comments about locking on dead on, but I still feel there's a place for a certain form of locking. The big thing is to understand that it won't be perfect, and then find a way to make it so perfection isn't needed.

The big question is why lock something. In most case it's to prevent a cache stampede (the dog pile effect) by making it so only a single process is regenerating it. If this is the use case trying to be solved then locking is a valid option- the worst case scenario is that your imperfect lock gets missed, causing a few additional instances to rebuild the same data. The alternative to that is having all of the instances running reprocess the data, so providing an imperfect lock still has it's benefits.

Additionally some of the other problems you brought up are easy to mitigate. The problems mentioned above really only apply to exclusive locks, which aren't the only kind. It's possible to put in a write lock while still allowing reads, letting other processes use the old data without allowing them to write new. You can also tell the caching system to use a default when it's locked, or (and this is my personal favorite) actually have the item pregenerate several minutes before it expires.

As long as locks are optional, and the developer has control over how to respond to a lock, then they have some serious benefits.

@Baachi Baachi commented on an outdated diff Nov 13, 2012
src/Symfony/Component/Cache/Driver/Apc.php
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Cache\Driver;
+
+/**
+ * This is our APC cache driver implementation
+ *
+ * @author Florin Patan <florinpatan@gmail.com>
+ */
+class Apc implements BatchDriverInterface
@Baachi
Baachi added a line comment Nov 13, 2012

I would suffix the classname with Driver.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dlsniper

@stof would you mind if I'd squash this a bit? I don't really like a long list of commits.

@Baachi

I vote for integrate a existing cache library, like stash. I hate to reinvent the wheel. Symfony2 integrates already some great libraries like monolog, doctrine/common, twig and so on.

But i must say, i don't like the design from stash. The implementation with the Pool instance is a little bit "weird".
And the coding style need some love. But this things can be fixed.

Is there any reason, why we don't integrate the zend/cache library?

@tedivm

@Baachi what about the Pool don't you like? It wasn't in the original versions of the library, but over time became more and more obvious that it had a real need. That being said, most people using Symfony won't see it directly, as they'll call cache objects out of a service (you can see this in the still very alpha bundle for Stash).

@dlsniper

@Baachi if you look at one of the previous threads, zend/cache was proposed then dismissed as having too many dependencies on other Zend libraries.

While the codding in Stash isn't a problem, a quick run with a IDE can fix the style, the other thing that you mention, the cache pool is something that @tedivm should help you with.

And like I've said earlier, when Fabien decides what we should have for Symfony2, I'll either close this or migrate it to what's decided.

@dlsniper

I see that @tedivm already replied. Can you tell us why that cache poll was a real need? I'm having troubles understanding it myself as it's not a approach I've usually seen. Thanks!

@tedivm

@dlsniper I'll write something up tomorrow- I'm just about to head to bed here. As a summary though giving the Cache Item a sense of "state" (as in, it's associated with a specific object and will remember about it between calls, as opposed to the driver method where each operation is completely independent) reduces multiple race conditions, makes it possible to return false and null as cached objects with no difficulty at all.

In other words, the real value that the Pool brings is allowing each Item to be associated with a specific Key. I'll try to write a more in depth set of reasoning over the next couple days (or just cobble together the points from the PSR discussion).

@dlsniper

@fabpot any thoughts on how should Symfony2 has the caching organized? I'm asking this because December is coming and most likely everyone will start being either away or very busy/away so there will be little time to take the decision and have the solution implemented / find every points that need to be analyzed/covered and tested in order to have something stable-ish by the end of this year.

I'll state again, I'm not attached to any particular solution as long as we have one and is implemented everywhere.

@tedivm

Although those lines, I should point out that substantial progress has been made with bringing Stash up to line. I've release the v0.10.* line which contains the API recommendations I've received, as well as substantial amounts of my own ideas.

From my perspective the next step in there is to get feedback from Symfony project members about anything they feel needs to be changed. I'll incorporate those changes in, and either release more in the v0.10.* line or bump up to v0.11 if any are backwards incompatible.

At the same time, the Stash Bundle has made some progress (although that was on hold for the other API changes). It's test suite runs, but still needs to be fleshed out. We have some decent debugging built in, including a nice collector for data for the Symfony admin panel. We've made some adapters- a Session adapter and a Doctrine adapter- and more are on their way to cover other services. Once we have the adapters made and working with the individual projects, I'd like to work with them on incorporating some changes to make caching more efficient and possibly have a single unified interface (allowing others to more easily build out caching libraries without needing multiple adaptors).

I know a decision has not been made about whether or not to use Stash, but I'll continue making progress on the library and am happy to incorporate relevant changes that make Stash better regardless of whether it becomes the default caching library, so please keep the feedback coming!

@lsmith77

@dlsniper i really urge you to involve yourself in the discussions on the FIG mailinglist. think it would be huge pitty if we push through something for just Symfony2.

@dlsniper

@lsmith77 I think that what I've implemented in here is a bit different that the existing proposals. Now, I'm not sure if I'm allowed to open up a new proposal, or even if I should open up a new one but I do believe that caching should be a simple, quick thing and that's the philosophy I've tried to have here.

Maybe in this regards, evert's proposal would be closer to this and what I'm trying to achieve.

What do you think the best approach would be?

@lsmith77

anyone can bring up a proposal .. but step one is to involve yourself in the discussion. there is always the chance that you will change other peoples minds, or you will change your own mind or both. but if you stay on the sidelines nothing will happen.

@dlsniper dlsniper referenced this pull request in php-fig/fig-standards Dec 1, 2012
Closed

Yet another cache proposal #63

@dlsniper

@fabpot should we mark this topic for 2.4? as it seems that FIG currently has long issues about naming things/placeholders rather that focusing even on bigger problems like logger, events, cache and so on, don't take it a rant.

I say 2.4 because, 2.3 iirc shouldn't introduce new things/major refactoring and things like HttpCache refactoring, proposed in #6213, could definitely benefit from this component so we either rush the implementation starting now either way for 2.4, with later making more sense if we want to be compatible with FIG decision or just provide a bridge (not that good) for that.

@fabpot
Symfony member

Closing as there are 4 opened PR on this topic. I would prefer that we first discuss the interface and what we want before starting coding. I suggest that this discussion happens on the Symfony dev mailing-list. Also, we need to discuss what to do with the current discussion on the FIG group mailing-list.

@fabpot fabpot closed this Mar 23, 2013
@noisebleed noisebleed referenced this pull request in ppi/framework Apr 22, 2013
Closed

Cache component #78

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment