Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[DI][Config] Add ContainerBuilder::classExists() for checking and tracking class, interface or trait existence#21452
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
| * | ||
| * @final | ||
| */ | ||
| public function getClassExists($class) |
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'm not sure this is the best name for this method. What aboutregisterClass() ortrackClass() or something like that.
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.
ok for a better name, but it should convey the fact that the return value has a meaning
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.
Hard but I agree,::trackedClassExists()... ?
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.
In fact, tracking is a side effect, so I'm not sure the name should convey that. When a method logs, we don't call it doThatAndLogIt
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.
So why not justdoesClassExist/classExists?
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 was going to proposeisTracked() ... but if tracking is not important...
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.
It's not that it's not important : it's a side effect. A conditional one also. What matters most is that this returns if a class exists or not.
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.
renamed to classExists
2839a7e to6140a26Compare| $this->addResource(new ClassExistenceResource($class)); | ||
| $this->classReflectors[$class] = false; | ||
| } else { | ||
| $class = $this->classReflectors[$class] = new \ReflectionClass($class); |
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 don't need to store the reflection class, we can storetrue instead, right ?
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 do, preparing#21419
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.
@nicolas-grekas wouldn't this hurt here as we create the reflector without adding a reflection resource, and the getter for the class reflector would not add it later as it is already there in the array
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.
that could be an issue, but the code in#21419 is free from it, see
https://github.com/symfony/symfony/pull/21419/files#diff-a10c9afd8feaccd1ff66ffd54e2835bcR356
The reflection resource is always added, even if the cache is hit. This creates duplicate resources, but they are free, and deduplicated later on so not an issue.
| * | ||
| * @return bool | ||
| * | ||
| * @final |
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.
Why using the annotation?
nicolas-grekasJan 30, 2017 • 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.
becausefinal is a non by-passable imperative keyword, and I don't like it at all for this reason.@final is way enough to press the intent. We don't need to be dictatorial if people want to do clever things like lazy proxies.
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.
Then imo we should have a solution that wouldn't throw a deprecation when used as a proxy. What about checking for a keyword such asmockable and don't throw a deprecation when common proxy interfaces are implemented?
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.
in fact, we already have one: proxies are not loaded via standard autoloading, so they don't get through DebugClassLoader :)
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.
Ok I did not know that, it's logic indeed :)
| @@ -65,6 +66,9 @@ private function updateDefinition(ContainerBuilder $container, $id, Definition $ | |||
| if (is_string($factory)) { | |||
| try { | |||
| $m = new \ReflectionFunction($factory); | |||
| if (false !== $m->getFileName() && file_exists($m->getFileName())) { | |||
| $container->addResource(new FileResource($m->getFileName())); | |||
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 we use your new reflection resource instead, to invalidate less often ?
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.
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.
If you mean for the function, we don't have a ReflectionFunction resource, and I don't think we need one.
Some suggestions innicolas-grekas#10 |
6140a26 toceddcd2Compare…tracking class, interface or trait existence
ceddcd2 to506bba8Comparenicolas-grekas commentedJan 30, 2017 • 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.
PR is ready, with all BUT I'm wondering if the benefit might be just DX, because checking a bunch of classes for existence takes time (even if this PR makes the situation a bit better by using only WDYT? |
We talked about My use case was with projects that don't have |
…ekas)This PR was merged into the 3.3-dev branch.Discussion----------[DI] Remove usages of ClassExistenceResource| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -As discussed in#21452 (see last comments)Commits-------ec8f1ad [DI] Remove usages of ClassExistenceResource
#21505 is the answer to this, right ? |
yes! |
Ans also to#21419 (comment) |
Subset of#21419