Introduce MetadataValidators to test arbitrary metadata for freshness. #15692

bnw · 2015-09-04T14:35:32Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#7230
Related ticket	#7176
License	MIT
Doc PR	TODO

This PR implements the idea discussed here: https://github.com/symfony/AsseticBundle/issues/168#issuecomment-13608074

Idea

Currently, any metadata that is passed to ConfigCache (namely implementations of ResourceInterface) is serialized to disk. Upon freshness check, it is unserialized and then asked, whether it is fresh.
The metadata has no chance to interact with any services, allowing only for very basic freshness checks.

With this PR, metadata no longer needs to implement the check for freshness itself. Instead, the ConfigCache::isFresh gets handed an array of MetadataValidator that take over the job of checking for freshness.
Each validator supports certain types of metadata. The implementations of ResourceInterface are now only a special type of metadata, that is validated using the ResourceInterfaceValidator.

In most cases, the ConfigCache should be used via the ConfigCacheFactory. I added a config_cache_factory service to which one can add validators via tagged services.

BC

Calling ConfigCache::isFresh without arguments is now deprecated.
Using ConfigCacheFactory without adding validators is deprecated.
However, both are still supported and yield the same results as before.

Added test for ResourceInterfaceValidator ConfigCache::isFresh: Stop validation as soon as we found a supporting validator

mpdude · 2015-09-04T14:54:55Z

The PR header says BC break yes / deprecations no. Is that correct?

linaori · 2015-09-04T18:25:05Z

src/Symfony/Component/Config/MetadataValidatorInterface.php

+    public function supports($metadata);
+
+    public function isFresh($metadata, $timestamp);
+


Missing docs for those 2 methods

…erspective)

ogizanagi · 2015-09-06T09:30:30Z

src/Symfony/Component/Config/ValidatorConfigCacheFactory.php

+ *
+ * @author Matthias Pigulla <mp@webfactory.de>
+ */
+class ValidatorConfigCacheFactory implements ConfigCacheFactoryInterface


Why creating a whole new config cache factory given adding validators to the old one is doable without BC break ? (But will prevent the deprecation due to https://github.com/symfony/symfony/pull/15692/files#diff-9f40590069e1fa6582e56bcea49e09a4R77)
Will the ConfigCacheFactory trigger a deprecation saying to use the new ValidatorConfigCacheFactory instead ?

For BC reasons, the Router and Translator need to create a ConfigCacheFactory themselves if none is injected. If that factory comes without the default ResourceInterfaceValidator, they'd have to add one themselves and that would cause unnecessary dependencies. Having a default ResourceInterfaceValidator in ConfigCacheFactory would be weird as well.

So this way, use a ConfigCacheFactory if you don't care about special validations and you'll get the same behaviour as in the past.

Use a ValidatorConfigCacheFactory if you care about special validations or if you get the instance injected anyway and don't care about the hassle of setting it up.

…n GH

…and unlikely to disappear in the future

This may have worked in the past, but was based on assumptions how the cache was implemented. You cannot dump a cache in prod and then expect it to be fresh or not when switching back to dev. A useful test would: 1. Use a mock loader to detect reloads 2. In prod: Use a Translator to prime the cache; try again with another instance and see that the cache is used 3. In dev: Use a Translator to prime the cache; add an "always stale" resource; try again with another instance and see that the cache is refreshed.

…e cache works internally

…e a rich man already

stof · 2015-09-08T08:10:00Z

src/Symfony/Component/Config/ConfigCache.php

+     * The first MetadataValidator that supports a given resource is considered authoritative.
+     * Resources with no matching MetadataValidators will silently be ignored and considered fresh.
+     *
+     * This method <em>does not</em> take the debug flag into consideration: Whether or not a cache


this looks weird to me. We need to take the debug flag into account

I'd like to make that a decision of the client (the ConfigCacheFactory). In production, it may skip all freshness checks or use a special set of Validators only.

Then, the cache itself would not need to know about debug at all.

…in the constructor. That way, we can keep the isFresh() method and get away without deprecations.

stof · 2015-09-08T12:16:15Z

src/Symfony/Component/Config/MetadataValidatorInterface.php

+ *
+ * @author Benjamin Klotz <bk@webfactory.de>
+ */
+interface MetadataValidatorInterface


I'm not sure MetadataValidator is the best name. This is more a resource checker than a validator

I'm open for naming suggestions.

CacheChecker

CacheMetadataChecker

MetadataChecker

ResourceChecker

ResourceMetadataChecker

...?

I would say ResourceChecker

ResourceCheckerInterface, that is?

for the interface, yes

stof · 2015-09-08T12:27:34Z

as discussed in #15719, I think we should keep a new ResourceInterface for resources instead of using object in the MetadataValidator. And this new interface should be accepted in places using resources in Symfony

mpdude · 2015-09-08T13:44:08Z

Maybe I have a completely wrong picture of what the transition path for such changes must look like.

For example, the MessageCatalogueInterface is @api and contains an addResource(ResourceInterface ...) method. I don't see how we could reasonably switch to a new resource interface there? We must have a path (with E_USER_DEPRECATED messages if possible?) over 2.8, right?

This is why I am trying to clean up the non-@api ResourceInterface (see #15719).

But if you can show me the right way to introduce a new interface for resources, I'd be glad to give it a try.

mpdude · 2015-09-08T19:13:21Z

What about this plan:

Add priorities so that new custom ResourceCheckers can be run first
Provide a default ResourceChecker with a very low priority that will validate through ResourceInterface::isFresh() but triggers a deprecation warning.
Add a new interface that extends ResourceInterface and contains the isFresh() method.
Add a ResourceChecker for this interface with a slightly higher priority and without deprecation warning.
Make FileResource et. al. implement the new interface.
Deprecate ResourceInterface::isFresh() but keep that method in the new interface subclass.
Also deprecate ResourceInterface::getResource() as per Deprecate ResourceInterface::getResource() #15719

We'll end up with ResourceInterface containing only __toString() in 3.0. Users implementing ResourceInterface will get a deprecation notice and can either switch to the new interface (no change beyond changing the "implements" list) or add a custom ResourceChecker to resolve it.

I acknowledge that @stof is in favor of having the new interface extend \Serializable as well, but I am not sure if that is possible in a BC way and still think that technically it is not necessary (i. e. should not stop us from getting all the rest).

* Add priorities so that new custom ResourceCheckers can be run first * Provide a default ResourceChecker with a very low priority that will validate through ResourceInterface::isFresh() but triggers a deprecation warning. * Add a new interface that extends ResourceInterface and contains the isFresh() method. * Add a ResourceChecker for this interface with a slightly higher priority and without deprecation warning. * Make FileResource et. al. implement the new interface. * Deprecate ResourceInterface::isFresh() but keep that method in the new interface subclass.

mpdude · 2015-09-09T12:46:06Z

I have implemented the above plan in #15738 and would like to close this PR in favor of it.

(I did not open this PR myself so I cannot close it.)

mpdude · 2015-09-16T07:36:04Z

@bnw could you please close this?

…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

bnw added 10 commits September 4, 2015 15:19

Introduce MetadataValidators to test arbitrary metadata for freshness.

60b4089

Added trailing newline

673070b

Added new parameter to interface

ab39129

Renamed ResourceValidator to ResourceInterfaceValidator

9546be9

Added test for ResourceInterfaceValidator ConfigCache::isFresh: Stop validation as soon as we found a supporting validator

Added missing import

0342b73

Correcting classname after renaming

7aee891

Added ConfigCachePass to framework

df526c0

Fixed UnitTests

2555d5a

Bugfix

4145dc0

Better name

2e49fb2

carsonbot added the Status: Needs Review label Sep 4, 2015

linaori reviewed Sep 4, 2015
View reviewed changes

mpdude added 7 commits September 5, 2015 10:35

Fix test

d0f1e2d

Add docblock

3f9424f

Fix tests while keeping ConfigCacheFactory unchanged (from a client p…

b8bb60a

…erspective)

Hello Fabbot

2fac8f3

Forgot import statement

1b62130

The new ConfigCachePass is required

b16d9de

Bump deps

97a2cd0

ogizanagi reviewed Sep 6, 2015
View reviewed changes

mpdude added 4 commits September 6, 2015 23:01

Avoid changing the interface

aae0a78

Also validate this method in the interface

024aa8c

Rename ResourceInterfaceValidator to ResourceValidator as suggested o…

d617943

…n GH

Oh Fabbot

8907a21

mpdude mentioned this pull request Sep 7, 2015

[Config] RFC - Simplify and redesign ResourceInterface #7176

Closed

mpdude added 2 commits September 7, 2015 10:34

Add a note why the ResourceInterface::__toString method is important …

e471890

…and unlikely to disappear in the future

mpdude added 3 commits September 7, 2015 13:23

Rewrite ConfigCacheTest because it made many assumptions about how th…

53ad659

…e cache works internally

Fix ConfigCache and improve docblock

594529c

If I had to pay just $0.02 for every Fabbot complaint, Fabien would b…

32426ce

…e a rich man already

mpdude mentioned this pull request Sep 7, 2015

Write a short cookbook entry how to make use of the ConfigCacheFactory in 2.7 symfony/symfony-docs#5136

Closed

mpdude added 3 commits September 7, 2015 21:41

Document deprecation in the CHANGELOG file

f3470e3

Some more tweaks to the ConfigCacheTest

7739878

Remove unused import

f9c48a0

mpdude mentioned this pull request Sep 8, 2015

Deprecate ResourceInterface::getResource() #15719

Merged

stof reviewed Sep 8, 2015
View reviewed changes

mpdude added 5 commits September 8, 2015 10:30

Continue with next resource, not next validator

c0f416d

Add a shortcut for when we don't have any validators (might be in prod)

00b4efb

Create a new ValidatorConfigCache class and pass metadata validators …

23a4464

…in the constructor. That way, we can keep the isFresh() method and get away without deprecations.

Fabbot

caf6bf1

This is not relevant for this PR

37c96e1

stof reviewed Sep 8, 2015
View reviewed changes

Tweaks as suggested on GH

7bc5ba2

mpdude mentioned this pull request Sep 9, 2015

Implement service-based Resource (cache) validation #15738

Closed

bnw closed this Sep 16, 2015

mpdude deleted the config-cache-metadata-validators branch September 24, 2015 19:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce MetadataValidators to test arbitrary metadata for freshness. #15692

Introduce MetadataValidators to test arbitrary metadata for freshness. #15692

bnw commented Sep 4, 2015

mpdude commented Sep 4, 2015

linaori Sep 4, 2015

ogizanagi Sep 6, 2015

mpdude Sep 7, 2015

stof Sep 8, 2015

mpdude Sep 8, 2015

stof Sep 8, 2015

mpdude Sep 8, 2015

stof Sep 8, 2015

mpdude Sep 8, 2015

stof Sep 8, 2015

stof commented Sep 8, 2015

mpdude commented Sep 8, 2015

mpdude commented Sep 8, 2015

mpdude commented Sep 9, 2015

mpdude commented Sep 16, 2015

		public function supports($metadata);

		public function isFresh($metadata, $timestamp);

Introduce MetadataValidators to test arbitrary metadata for freshness. #15692

Introduce MetadataValidators to test arbitrary metadata for freshness. #15692

Conversation

bnw commented Sep 4, 2015

Idea

BC

mpdude commented Sep 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Sep 8, 2015

mpdude commented Sep 8, 2015

mpdude commented Sep 8, 2015

mpdude commented Sep 9, 2015

mpdude commented Sep 16, 2015