[Config] Allow for service-based Resource (cache) validation #7230
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -1,5 +1,4 @@ | |||
| <?php | |||
|
|
|||
fabpot
added a commit
that referenced
this pull request
Mar 14, 2013
This PR was merged into the 2.1 branch. Commits ------- f2ef6bc [FrameworkBundle] removed BC break cc3a40e [FrameworkBundle] changed temp kernel name in cache:clear 7d87ecd [FrameworkBundle] fixed cahe:clear command's warmup Discussion ---------- [FrameworkBundle] fixes cahe:clear command's warmup Solution taken is to replace the last char of the cache directory name to create a temporary cache directory, this way the temporary cache path has the same length than the real one. I tested this on several projects, in dev and prod environments. | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | #6203 --------------------------------------------------------------------------- by jfsimon at 2013-03-13T12:32:25Z @toloco @gergelypolonkai @ghost-x47 @stewe it would be great if you could test this patch on your projects and report result! --------------------------------------------------------------------------- by toloco at 2013-03-13T12:41:47Z Im sorry but have the same... Notice: unserialize(): Error at offset 155 of 174227 bytes in /home/tolopalmer/Projects/shareandcoach/app/bootstrap.php.cache line 915 --------------------------------------------------------------------------- by jfsimon at 2013-03-13T12:45:04Z @toloco could you paste the backtrace in a gist? and maybe the concerned file? --------------------------------------------------------------------------- by stof at 2013-03-13T13:11:47Z @jfsimon You probably have the same issue with the name of the temporary kernel class --------------------------------------------------------------------------- by jfsimon at 2013-03-13T13:36:13Z @stof if you're right, it's a nightmare. It must be possible to write a parser/fixer for serialized objects, don't you think? --------------------------------------------------------------------------- by toloco at 2013-03-13T14:22:56Z Here you are the gist with the stack and the bootstrap.php.cache file https://gist.github.com/toloco/5152581 --------------------------------------------------------------------------- by mpdude at 2013-03-13T20:08:08Z @jfsimon Writing such a parser is painting yourself in the corner. Use a temp kernel class name of the same length as a quick fix. #7230 could bring a solution because we might be able to inject a different ConfigCache factory during the command that intercepts and substitutes Resources before they get written into the meta file. Not sure if that PR has a chance of being picked though. --------------------------------------------------------------------------- by toloco at 2013-03-14T08:19:58Z So guys? we are blocked with this problem, can I help you? I can provide more stacks if it's needed --------------------------------------------------------------------------- by mpdude at 2013-03-14T10:05:05Z @toloco Could you please post the /home/tolopalmer/Projects/shareandcoach/app/cache/dev/appDevUrlMatcher.php.meta file? That's the one that is broken. --------------------------------------------------------------------------- by jfsimon at 2013-03-14T10:15:20Z @mpdude you can find its content in the gist https://gist.github.com/toloco/5152581 (1st file, 6th line) --------------------------------------------------------------------------- by mpdude at 2013-03-14T10:24:55Z @toloco That file should contain a serialized set of Resources, it's not in the Gist. --------------------------------------------------------------------------- by jfsimon at 2013-03-14T10:33:12Z @mpdude it's more visible in the raw file: ttps://gist.github.com/toloco/5152581/raw/48a1a823b5c8e6ba03936a52e8dc0d0ff1888f8a/Error+ --------------------------------------------------------------------------- by jfsimon at 2013-03-14T10:33:27Z sorry: https://gist.github.com/toloco/5152581/raw/48a1a823b5c8e6ba03936a52e8dc0d0ff1888f8a/Error+ --------------------------------------------------------------------------- by toloco at 2013-03-14T10:37:09Z https://gist.github.com/toloco/5160317 here you are the appDevUrlMatcher.php and meta --------------------------------------------------------------------------- by jfsimon at 2013-03-14T10:51:46Z @toloco I applied @mpdude's solution (have a temp kernel class name of the same length than the real one). Could you test it to see if it fixes your problem? --------------------------------------------------------------------------- by mpdude at 2013-03-14T10:58:46Z @jfsimon Thanks! @toloco If Jean-François' fix does not work, please make sure that the .meta file you posted was the broken one? I was able to unserialize it without problems. --------------------------------------------------------------------------- by toloco at 2013-03-14T11:02:09Z Man!!!! you are the fucking boss it works!! --------------------------------------------------------------------------- by mpdude at 2013-03-14T11:04:30Z @jfsimon you just made someone happy. --------------------------------------------------------------------------- by jfsimon at 2013-03-14T11:12:39Z @toloco @mpdude \o/
…ory altogether, as Router and Translator will still fall back to old behaviour (BC).
It is a common pattern for clients of ConfigCache/ConfigCacheFactory to check whether the cache is fresh and if not, fill it. This removes a little bit of repeated code. Additionally, it gives factory implementations more control over how the cache is written to (for example they can employ other file locking/atomic write strategies).
That way, we can get consistent results even when files not matching a pattern are added or removed. A change to the directory alone won't change the hash.
…f ConfigCache. They never need to deal with it directly, but only carry it to pass it along to a ConfigCache later on. Thus, using array_unique is an optimization only relevant in the cache implementation.
…rkBundle specific factory
…d File/Directory resource. If client classes collecting resources (like RouteCollection or MessageCatalogue) accept arbitrary objects and not only ResourceInterface instances, then we can leave ResourceInterface, FileResource and DirectoryResource alone. Also, we don't need the SelfValidatingResourceInterface but can instead provide a ResourceInterfaceValidator that is able to validate all ResourceInterface instances.
|
Instead, arbitrary objects are accepted as Resources in the This is necessary because for some kinds of Resources (e. g. database-backed) it is not possible to meaningfully implement The arbitrary Resource objects are passed around to all Validators and only those Resources a Validator knows/recognizes are kept and used for validation. |
|
@fabpot Would it help (make reviewing easier) if I break this down into smaller PRs? To solve the bugs mentioned on top it would be necessary to have most of the stuff here, but I think I can carve out subsets that would at least allow to get some of the benefit by implementing the rest in userland. |
|
Indeed, smaller PRs would make things easier for me. Each time I come back to this PR, I have a hard figuring out where to start first. I have one important comment about the current state of this patch: breaking BC is not an option anymore. So, for instance, you cannot change the method signature for the ContainerBuilder and other classes (where you have removed the |
|
Does that apply to all methods or just @api stuff? |
|
At this stage, it applies to almost all public methods. For non-@api tagged public methods, we might break BC if this is really required and if it is often used by end users. |
1 task
fabpot
added a commit
that referenced
this pull request
Apr 21, 2013
This PR was squashed before being merged into the master branch (closes #7753). Discussion ---------- Mitigate dependency upon ConfigCache | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes (refactoring) | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | todo, issue is symfony/symfony-docs#2531 Some clients use ConfigCache only for its ability to atomically write into a file. The PR moves that code into the Filesystem component. It's a pick off #7230. __To-Do:__ - [ ] Update docs for the Filesystem component Commits ------- 3158c41 Mitigate dependency upon ConfigCache
This was referenced Apr 21, 2013
|
Just to make sure this leads us somewhere. We can keep the ResourceInterface as-is. For some special cases clients might need to no-op implement it, but they can have "their" interface alongside it. Still the ResourceValidators can be used as a place where unserialize()d resources get in touch with a service prior to the isFresh() check so f.e. a FileLocator can be injected. I understand that API changes are not an option. |
| $name = " ($name)"; | ||
| } | ||
|
|
||
| if (!is_dir($dir)) { |
There was a problem hiding this comment.
Maybe its better to use the symfon2 Filesystem component.
1 task
This was referenced Apr 2, 2015
fabpot
added a commit
that referenced
this pull request
Apr 8, 2015
… a factory. (mpdude) This PR was squashed before being merged into the 2.7 branch (closes #14178). Discussion ---------- [Config] Delegate creation of ConfigCache instances to a factory. | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes (refactoring, new flex point) | BC breaks? | no | Deprecations? | yes | Tests pass? | we'll see :-) | Fixed tickets | n/a | License | MIT | Doc PR | symfony/symfony-docs#5136 In the Routing/Router and Translation/Translator, delegate creation of ConfigCache instances to a factory. The factory can be setter-injected but will default to a BC implementation. The ```ConfigCacheFactoryInterface``` is designed in a way that captures the common ```$cache = new ...; if (!$cache->isFresh()) { ... do sth }``` pattern. But more importantly, this design allows factory implementations to take additional measures to avoid race conditions before actually filling the cache. By using an exchangeable ConfigCache factory it becomes possible to implement different resource (freshness) checking strategies, especially service-based ones. The goal is to be able to validate Translators and Routers generated by database-based loaders. It might also help with symfony/AsseticBundle#168. This PR only contains the minimum changes needed, so the rest could be implemented in a bundle outside the core (at least for the beginning). Component/HttpKernel/Kernel::initializeContainer still uses the ConfigCache implementation directly as there is no sensible way of getting/injecting a factory service (chicken-egg). This is a pick off #7230. It replaces #7781 which was against the master branch. Also see #7781 for additional comments/explanations. ## Todo * [ ] Allow `symfony/config` `~3.0.0` in `composer.json` for the HttpKernel and Translator component as well as TwigBundle once this PR has been merged into the master branch (fail deps=high tests for the time being). Commits ------- 6fbe9b1 [Config] Delegate creation of ConfigCache instances to a factory.
fabpot
added a commit
that referenced
this pull request
Sep 25, 2015
…pdude) This PR was squashed before being merged into the 2.8 branch (closes #15738). Discussion ---------- Implement service-based Resource (cache) validation | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #7230, #15692, #7782 | License | MIT | Doc PR | symfony/symfony-docs#5136 ### Overview Currently, any metadata passed to `ConfigCache` (namely implementations of `ResourceInterface`) is serialized to disk. When the `ConfigCache` is validated, the metadata is unserialized and queried through `ResourceInterface::isFresh()` to determine whether the cache is fresh. That way, `ResourceInterface` implementations cannot interact with services, for example a database connection. This PR introduces the new concept of `ResourceCheckers`. Services implementing `ResourceCheckerInterface` can be tagged as `config_cache.resource_checker` with an optional priority. Clients that wish to use `ConfigCache` can then obtain an instance from the `config_cache_factory` service (which implements `ConfigCacheFactoryInterface`). The factory will take care of injecting resource checkers into the `ConfigCache` instance so that they can be used for cache validation. Checking cache metadata is easy for `ResourceCheckers`: * First, the `ResourceCheckerInterface::supports()` implementation is passed the metadata object in question. If the checker cannot handle the type of resource passed, `supports()` should return `false`. * Otherwise, the `ResourceCheckerInterface::isFresh()` method will be called and given the resource as well as the timestamp at which the cache was initialized. If that method returns `false`, the cache is considered stale. If it returns `true`, the resource is considered unchanged and will *not* be passed to any additional checkers. ### BC and migration path This PR does not (intend to) break BC but it comes with deprecations. The main reason is that `ResourceInterface` contains an `isFresh()` method that does not make sense in the general case of resources. Thus, `ResourceInterface::isFresh()` is marked as deprecated and should be removed in Symfony 3.0. Resource implementations that can (or wish to) be validated in that simple manner can implement the `SelfCheckingResourceInterface` sub-interface that still contains (and will keep) the `isFresh()` method. The change should be as simple as changing the `extends` list. Apart from that, `ResourceInterface` will be kept as the base interface for resource implementations. It is used in several `@api` interfaces and thus cannot easily be substituted. For the Symfony 2.x series, a `BCResourceInterfaceChecker` will be kept that performs validation through `ResourceInterface::isFresh()` but will trigger a deprecation warning. The remedy is to either implement a custom ResourceChecker with a priority higher than -1000; or to switch to the aforementioned `SelfCheckingResourceInterface` which is used at a priority of -990 (without deprecation warning). The `ConfigCache` and `ConfigCacheFactory` classes can be used as previously but do not feature checker-based cache validation. ### Outlook and closing remarks: This PR supersedes #7230, #15692 and works at least in parts towards the goal of #7176. The `ResourceCheckerInterface`, `...ConfigCache` and `...ConfigCacheFactory` no longer need to be aware of the `debug` flag. The different validation rules applied previously are now just a matter of `ResourceChecker` configuration (i. e. "no checkers" in `prod`). It might be possible to remove the `debug` flag from Symfony's `Router` and/or `Translator` classes in the future as well because it was only passed on to the `ConfigCache` there. Commits ------- 20d3722 Implement service-based Resource (cache) validation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closed PR: #15738 is the current attempt to solve this.
This PR adds the possibility to use services for Resource checking. It brings along a flex point for changing cache implementations.
Motivation
The Loader pattern applied in various places is very powerful, as it does not only abstract the format (YAML vs. XML, for example) of a file; it is equally well suited to fetch configuration/data from elsewhere. To give just one example, translations can easily be fetched from a database.
Alongside Loaders, ConfigCache and the Resource interface are often used to speedup the loading and processing of configuration.
Implementing custom Resources, for example for the said database holding translations, is not so easy because currently the Resources are serialized, kept with the cache, unserialized later on and used to check if the cache is still fresh.
This check has to be performed by the Resource alone which has just been unserialized. That works sufficiently well as long as Resources merely aim at files and need to check a filemtime() or similar. But Resources have no clean way to access services (like a database connection).
Summary of changes
This PR shifts the responsibility for checking Resource freshness from the ([un]serialized) Resource instance to instances of a newly introduced
ResourceValidatorInterface. Resources may become merely values that just show the ResourceValidators what to validate.ResourceValidator instances can be declared as services and thus fully leverage the DIC. When using the FrameworkBundle, a DIC tag can be used to mark them.
In order to get a ConfigCache-implementation equipped with validators, client classes can no longer create instances themselves but should use a factory service (provided in the DIC).
This adds yet another interesting flex point, as it becomes possible to control which validators (or any at all) will be used for Resource validation. Currently, it's just all (in kernel.debug) or none (in production). With this flexibility added, client code is free to decide what is appropriate.
Remarks
ConfigCacheFactory, theRouterandTranslatoruse theDefaultConfigCacheFactory. That way, both can be used without a DIC and behave as previously.DefaultConfigCacheFactoryand delegates to the created cache instance. This is not a problem for clients using ConfigCache (creating instances of it), but probably breaks subclasses ofConfigCacheitself.To-Dos: