-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Config] Delegate creation of ConfigCache instances to a factory. #14178
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.
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
Changes from all commits
02159ae
98aa48b
3744edf
282a75a
ae29b82
1730fc3
770ee09
2a058fd
8491dfd
beb307c
dbec920
5a91716
f2c3a24
35488bd
17a55ec
07ecf57
a89db7a
7938349
cb9917f
2016537
beb0a92
afee2bb
4832e1a
7f90bae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,14 +23,12 @@ | |
* | ||
* @author Fabien Potencier <fabien@symfony.com> | ||
*/ | ||
class ConfigCache | ||
class ConfigCache implements ConfigCacheInterface | ||
{ | ||
private $debug; | ||
private $file; | ||
|
||
/** | ||
* Constructor. | ||
* | ||
* @param string $file The absolute cache path | ||
* @param bool $debug Whether debugging is enabled or not | ||
*/ | ||
|
@@ -44,8 +42,21 @@ public function __construct($file, $debug) | |
* Gets the cache file path. | ||
* | ||
* @return string The cache file path | ||
* @deprecated since 2.7, to be removed in 3.0. Use getPath() instead. | ||
*/ | ||
public function __toString() | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You need to add a deprecation notice (look for |
||
trigger_error('ConfigCache::__toString() is deprecated since version 2.7 and will be removed in 3.0. Use the getPath() method instead.', E_USER_DEPRECATED); | ||
|
||
return $this->file; | ||
} | ||
|
||
/** | ||
* Gets the cache file path. | ||
* | ||
* @return string The cache file path | ||
*/ | ||
public function getPath() | ||
{ | ||
return $this->file; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
<?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\Config; | ||
|
||
/** | ||
* Basic implementation for ConfigCacheFactoryInterface | ||
* that will simply create an instance of ConfigCache. | ||
* | ||
* @author Matthias Pigulla <mp@webfactory.de> | ||
*/ | ||
class ConfigCacheFactory implements ConfigCacheFactoryInterface | ||
{ | ||
/** | ||
* @var bool Debug flag passed to the ConfigCache | ||
*/ | ||
private $debug; | ||
|
||
/** | ||
* @param bool $debug The debug flag to pass to ConfigCache | ||
*/ | ||
public function __construct($debug) | ||
{ | ||
$this->debug = $debug; | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function cache($file, $callback) | ||
{ | ||
$cache = new ConfigCache($file, $this->debug); | ||
|
||
if (!$cache->isFresh()) { | ||
call_user_func($callback, $cache); | ||
} | ||
|
||
return $cache; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
<?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\Config; | ||
|
||
/** | ||
* Interface for a ConfigCache factory. This factory creates | ||
* an instance of ConfigCacheInterface and initializes the | ||
* cache if necessary. | ||
* | ||
* @author Matthias Pigulla <mp@webfactory.de> | ||
*/ | ||
interface ConfigCacheFactoryInterface | ||
{ | ||
/** | ||
* Creates a cache instance and (re-)initializes it if necessary. | ||
* | ||
* @param string $file The absolute cache file path | ||
* @param callable $callable The callable to be executed when the cache needs to be filled (i. e. is not fresh). The cache will be passed as the only parameter to this callback | ||
* | ||
* @return ConfigCacheInterface $configCache The cache instance | ||
*/ | ||
public function cache($file, $callable); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we need to change file argument name here and to make it more generic. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know... I did not intend to change the way |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
<?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\Config; | ||
|
||
use Symfony\Component\Config\Resource\ResourceInterface; | ||
|
||
/** | ||
* Interface for ConfigCache | ||
* | ||
* @author Matthias Pigulla <mp@webfactory.de> | ||
*/ | ||
interface ConfigCacheInterface | ||
{ | ||
/** | ||
* Gets the cache file path. | ||
* | ||
* @return string The cache file path | ||
*/ | ||
public function getPath(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ATM this only returns the path (replacing the |
||
|
||
/** | ||
* Checks if the cache is still fresh. | ||
* | ||
* This check should take the metadata passed to the write() method into consideration. | ||
* | ||
* @return bool Whether the cache is still fresh. | ||
*/ | ||
public function isFresh(); | ||
|
||
/** | ||
* Writes the given content into the cache file. Metadata will be stored | ||
* independently and can be used to check cache freshness at a later time. | ||
* | ||
* @param string $content The content to write into the cache | ||
* @param ResourceInterface[]|null $metadata An array of ResourceInterface instances | ||
* | ||
* @throws \RuntimeException When the cache file cannot be written | ||
*/ | ||
public function write($content, array $metadata = null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here for $content |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,7 +26,7 @@ | |
"symfony/phpunit-bridge": "~2.7|~3.0.0", | ||
"symfony/browser-kit": "~2.3|~3.0.0", | ||
"symfony/class-loader": "~2.1|~3.0.0", | ||
"symfony/config": "~2.0,>=2.0.5|~3.0.0", | ||
"symfony/config": "~2.7", | ||
"symfony/console": "~2.3|~3.0.0", | ||
"symfony/css-selector": "~2.0,>=2.0.5|~3.0.0", | ||
"symfony/dependency-injection": "~2.2|~3.0.0", | ||
|
@@ -40,6 +40,9 @@ | |
"symfony/translation": "~2.0,>=2.0.5|~3.0.0", | ||
"symfony/var-dumper": "~2.6|~3.0.0" | ||
}, | ||
"conflict": { | ||
"symfony/config": "<2.7" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need this line? The requirement should been enough, no? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The require-dev does not work if symfony/config is followed as a transitive dependency during dep=... builds. So There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @fabpot Do you prefer a PR with tests "all green", or shall I keep changes to a minimum at the cost of having dep= tests pass only once the change has been merged up to the master branch? I think this is a special case here because changes are made across components that are then tested independently. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, I thought |
||
}, | ||
"suggest": { | ||
"symfony/browser-kit": "", | ||
"symfony/class-loader": "", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deprecations don't belong in the UPGRADE file, instead it should be added in the CHANGELOG file of the Config component and the UPGRADE-3.0.md file should document that this method was removed from the interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure about deprecations? There are a lot of similar comments in UPGRADE-2.7 already.