Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
Class existence resource#20121
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
Class existence resource#20121
Uh oh!
There was an error while loading.Please reload this page.
Conversation
fabpot commentedOct 1, 2016
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | see#20094 |
| License | MIT |
| Doc PR | n/a |
9232ba4 to8d0ff06Compare| namespace Symfony\Component\Config\Resource; | ||
| /** | ||
| * ClassExistenceResource represents a class availability. |
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.
Let's replaceavailability withexistence, to be consistent with the class name and theclass_exists function name.
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.
done
| EOF | ||
| ); | ||
| $this->assertFalse($res->isFresh(time())); |
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.
Afaik, this should beassertTrue. In table form (horizontal: current state, vertical: stored state)
| Class exists | Class does not exists | |
|---|---|---|
| Class exists | Not fresh | Fresh |
| Class does not exists | Fresh | Not fresh |
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.
The freshness is all about the state change. The resource is fresh if there is no change in class existence, so no need to refresh the cache. Not fresh means that the cache must be recalculated, so when the state changes. So, the table should be:
| Class exists | Class does not exists | |
|---|---|---|
| Class exists | Fresh | Not fresh |
| Class does not exists | Not fresh | fresh |
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.
Oh, sorry. Completely messed up the definition of "fresh"...
| @@ -72,15 +76,18 @@ public function process(ContainerBuilder $container) | |||
| $container->getDefinition('twig.extension.assets')->addTag('twig.extension'); | |||
| } | |||
| if (class_exists('Symfony\Component\Yaml\Parser')) { | |||
| $container->addResource(new ClassExistenceResource(YamlParser::class)); | |||
| if (class_exists(YamlParser::class)) { | |||
javiereguiluzOct 2, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Related tothis comment, just asking if we could replace this:
if (class_exists(YamlParser::class)) {$container->getDefinition('twig.extension.yaml')->addTag('twig.extension');}
by this?
if (!class_exists(YamlParser::class)) {$container->removeDefinition('twig.extension.yaml');}
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.
The difference is that if one uses thetwig.extension.yaml directly, the proposed version will break.
I think the current approach is a more open thus a better practice, even if in this very case using the extension directly is so unlikely.
8d0ff06 to222b56dCompareThis PR was merged into the 3.2-dev branch.Discussion----------Class existence resource| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | see#20094| License | MIT| Doc PR | n/aCommits-------222b56d [TwigBundle] added support for ClassExistenceResource when relevantd98eb7b [Config] added ClassExistenceResource
This PR was merged into the 4.0.x-dev branch.Discussion----------added support for ClassExistenceResourcedepends onsymfony/symfony#20121Commits-------a1f987d added support for ClassExistenceResource
| public function __construct($resource) | ||
| { | ||
| $this->resource = $resource; | ||
| $this->exists = class_exists($resource); |
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.
we should useclass_exists() || interface_exists() || trait_exists() to make it usable for interfaces and traits too