Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Implement service-based Resource (cache) validation#15738
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
missing@ (which would make the testsuite fail btw)
fabpot commentedSep 14, 2015
The UPGRADE-2.8.md and 3.0 file should be updated with instruction on how to upgrade. |
mpdude commentedSep 14, 2015
I added entries in the UPGRADING files and hope that's what you expect. |
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 caceh 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, support() 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 valid 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 shall 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. Note that ResourceInterface 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.
…ngResourceInterface
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.
{ should be in next line.
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.
Hey FabBot, are you sleeping :)?
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.
Shouldn't this befindDefinition()?
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.
Well, that service is defined and not an alias. Do you think it should be one?
mpdude commentedSep 16, 2015
mpdude commentedSep 23, 2015
Build failure due to transient test on Windows, added to the list at#15617 (comment) |
mpdude commentedSep 23, 2015
ping@fabpot |
fabpot commentedSep 23, 2015
👍 |
fabpot commentedSep 24, 2015
ping @symfony/deciders |
dunglas commentedSep 24, 2015
👍 |
fabpot commentedSep 25, 2015
Thank you@mpdude. |
mpdude commentedSep 25, 2015
stof commentedSep 25, 2015
This PR is breaking tests |
mpdude commentedSep 25, 2015
Where? |
stof commentedSep 25, 2015
Ah, I understand the issue: this change is not yet merged into master, and |
fabpot commentedSep 25, 2015
done |
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.
any reason why this test fails ?
$translator->setConfigCacheFactory(new \Symfony\Component\Config\ResourceCheckerConfigCacheFactory([new \Symfony\Component\Config\Resource\SelfCheckingResourceChecker(),new \Symfony\Component\Config\Resource\BCResourceInterfaceChecker()]));
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.
Don't know, but maybe you want to use the default ConfigCacheFactory in that case?
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.
I usedResourceCheckerConfigCacheFactory, the problems occurs when we want to work with the debug mode, is there a way to pass debug mode forResourceCheckerConfigCacheFactory like theold one ?
Overview
Currently, any metadata passed to
ConfigCache(namely implementations ofResourceInterface) is serialized to disk. When theConfigCacheis validated, the metadata is unserialized and queried throughResourceInterface::isFresh()to determine whether the cache is fresh. That way,ResourceInterfaceimplementations cannot interact with services, for example a database connection.This PR introduces the new concept of
ResourceCheckers. Services implementingResourceCheckerInterfacecan be tagged asconfig_cache.resource_checkerwith an optional priority.Clients that wish to use
ConfigCachecan then obtain an instance from theconfig_cache_factoryservice (which implementsConfigCacheFactoryInterface). The factory will take care of injecting resource checkers into theConfigCacheinstance so that they can be used for cache validation.Checking cache metadata is easy for
ResourceCheckers:ResourceCheckerInterface::supports()implementation is passed the metadata object in question. If the checker cannot handle the type of resource passed,supports()should returnfalse.ResourceCheckerInterface::isFresh()method will be called and given the resource as well as the timestamp at which the cache was initialized. If that method returnsfalse, the cache is considered stale. If it returnstrue, the resource is considered unchanged and willnot 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
ResourceInterfacecontains anisFresh()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 theSelfCheckingResourceInterfacesub-interface that still contains (and will keep) theisFresh()method. The change should be as simple as changing theextendslist.Apart from that,
ResourceInterfacewill be kept as the base interface for resource implementations. It is used in several@apiinterfaces and thus cannot easily be substituted.For the Symfony 2.x series, a
BCResourceInterfaceCheckerwill be kept that performs validation throughResourceInterface::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 aforementionedSelfCheckingResourceInterfacewhich is used at a priority of -990 (without deprecation warning).The
ConfigCacheandConfigCacheFactoryclasses 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,...ConfigCacheand...ConfigCacheFactoryno longer need to be aware of thedebugflag. The different validation rules applied previously are now just a matter ofResourceCheckerconfiguration (i. e. "no checkers" inprod).It might be possible to remove the
debugflag from Symfony'sRouterand/orTranslatorclasses in the future as well because it was only passed on to theConfigCachethere.