[DI][Config] Add ContainerBuilder::classExists() for checking and tracking class, interface or trait existence #21452

nicolas-grekas · 2017-01-29T15:06:06Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	/no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

Subset of #21419

javiereguiluz · 2017-01-29T15:23:13Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

+     *
+     * @final
+     */
+    public function getClassExists($class)


I'm not sure this is the best name for this method. What about registerClass() or trackClass() or something like that.

ok for a better name, but it should convey the fact that the return value has a meaning

Hard but I agree, ::trackedClassExists()... ?

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

So why not just doesClassExist/classExists?

I was going to propose isTracked() ... but if tracking is not important...

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.

renamed to classExists

GuilhemN · 2017-01-30T10:06:44Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

+                $this->addResource(new ClassExistenceResource($class));
+                $this->classReflectors[$class] = false;
+            } else {
+                $class = $this->classReflectors[$class] = new \ReflectionClass($class);


We don't need to store the reflection class, we can store true instead, right ?

we do, preparing #21419

@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

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.

GuilhemN · 2017-01-30T10:09:18Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

+     *
+     * @return bool
+     *
+     * @final


Why using the annotation?

because final 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.

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 as mockable and don't throw a deprecation when common proxy interfaces are implemented?

in fact, we already have one: proxies are not loaded via standard autoloading, so they don't get through DebugClassLoader :)

Ok I did not know that, it's logic indeed :)

stof · 2017-01-30T10:19:46Z

src/Symfony/Component/DependencyInjection/Compiler/FactoryReturnTypePass.php

@@ -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()));


shouldn't we use your new reflection resource instead, to invalidate less often ?

yes, done in #21419 : https://github.com/symfony/symfony/pull/21419/files#diff-58acb002c7d222ac4537afd97530104bR85

If you mean for the function, we don't have a ReflectionFunction resource, and I don't think we need one.

chalasr · 2017-01-30T12:27:01Z

Some suggestions in nicolas-grekas#10

…tracking class, interface or trait existence

nicolas-grekas · 2017-01-30T14:34:22Z

PR is ready, with all class_exists() replaced by $container->classExists().

BUT

I'm wondering if ClassExistenceResource is required at all ~~and if we shouldn't deprecate it instead~~ (we should not deprecate it because #21419 uses it for userland classes - but still, we could not use it for deps related checks).
ping @fabpot because you added it in #20121
should we really need to track things that change only when composer is used?
shouldn't we instead expect people/"a composer script" to clear the dumped container when deps change?

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 FileExistenceResource when possible).

WDYT?

fabpot · 2017-01-30T19:52:49Z

We talked about ClassExistenceResource with @nicolas-grekas and I'm going to fix my edge case another way, so this class is not needed anymore for classes managed via Composer.

My use case was with projects that don't have symfony/console, so cache:clear is not run by Composer. That's mainly the case when using only the Symfony components with Flex, and I will find another way.

…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

stof · 2017-02-02T13:02:15Z

My use case was with projects that don't have symfony/console, so cache:clear is not run by Composer. That's mainly the case when using only the Symfony components with Flex, and I will find another way.

#21505 is the answer to this, right ?

nicolas-grekas · 2017-02-02T13:03:54Z

yes!

nicolas-grekas · 2017-02-02T13:04:26Z

Ans also to #21419 (comment)

carsonbot added Status: Needs Review DependencyInjection Config Feature labels Jan 29, 2017

nicolas-grekas added this to the 3.3 milestone Jan 29, 2017

This was referenced Jan 29, 2017

[DI][Config] Add & use ReflectionClassResource #21419

Merged

[DI] Add ContainerBuilder::fileExists() for checking/tracking resource existence #21408

Merged

javiereguiluz reviewed Jan 29, 2017

View reviewed changes

nicolas-grekas force-pushed the di-class-exists branch 3 times, most recently from 2839a7e to 6140a26 Compare January 29, 2017 18:42

nicolas-grekas changed the title ~~[DI][Config] Add ContainerBuilder::getClassExists() for checking and tracking class, interface or trait existence~~ [DI][Config] Add ContainerBuilder::classExists() for checking and tracking class, interface or trait existence Jan 29, 2017

GuilhemN reviewed Jan 30, 2017

View reviewed changes

stof reviewed Jan 30, 2017

View reviewed changes

nicolas-grekas force-pushed the di-class-exists branch from 6140a26 to ceddcd2 Compare January 30, 2017 14:25

[DI][Config] Add ContainerBuilder::getClassExists() for checking and …

506bba8

…tracking class, interface or trait existence

nicolas-grekas force-pushed the di-class-exists branch from ceddcd2 to 506bba8 Compare January 30, 2017 14:29

nicolas-grekas closed this Jan 30, 2017

nicolas-grekas deleted the di-class-exists branch January 30, 2017 20:54

nicolas-grekas mentioned this pull request Jan 30, 2017

[DI] Remove usages of ClassExistenceResource #21466

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI][Config] Add ContainerBuilder::classExists() for checking and tracking class, interface or trait existence #21452

[DI][Config] Add ContainerBuilder::classExists() for checking and tracking class, interface or trait existence #21452

nicolas-grekas commented Jan 29, 2017

javiereguiluz Jan 29, 2017

nicolas-grekas Jan 29, 2017

chalasr Jan 29, 2017

nicolas-grekas Jan 29, 2017

ogizanagi Jan 29, 2017

javiereguiluz Jan 29, 2017

nicolas-grekas Jan 29, 2017

nicolas-grekas Jan 29, 2017

GuilhemN Jan 30, 2017

nicolas-grekas Jan 30, 2017

stof Jan 30, 2017

nicolas-grekas Jan 30, 2017

GuilhemN Jan 30, 2017

nicolas-grekas Jan 30, 2017 •

edited

Loading

GuilhemN Jan 30, 2017

nicolas-grekas Jan 30, 2017

GuilhemN Jan 30, 2017 •

edited

Loading

stof Jan 30, 2017

nicolas-grekas Jan 30, 2017

nicolas-grekas Jan 30, 2017

chalasr commented Jan 30, 2017

nicolas-grekas commented Jan 30, 2017 •

edited

Loading

fabpot commented Jan 30, 2017

stof commented Feb 2, 2017

nicolas-grekas commented Feb 2, 2017

nicolas-grekas commented Feb 2, 2017

[DI][Config] Add ContainerBuilder::classExists() for checking and tracking class, interface or trait existence #21452

[DI][Config] Add ContainerBuilder::classExists() for checking and tracking class, interface or trait existence #21452

Conversation

nicolas-grekas commented Jan 29, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuilhemN Jan 30, 2017 • edited Loading

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

chalasr commented Jan 30, 2017

nicolas-grekas commented Jan 30, 2017 • edited Loading

fabpot commented Jan 30, 2017

stof commented Feb 2, 2017

nicolas-grekas commented Feb 2, 2017

nicolas-grekas commented Feb 2, 2017

nicolas-grekas Jan 30, 2017 •

edited

Loading

GuilhemN Jan 30, 2017 •

edited

Loading

nicolas-grekas commented Jan 30, 2017 •

edited

Loading