[ClassLoader] Fix storing not-found classes in APC cache #18312
Conversation
nicolas-grekas
commented
Mar 25, 2016
| Q | A |
|---|---|
| Branch? | 2.3 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | - |
| License | MIT |
| Doc PR | - |
|
Did this cause issues somewhere? The docs state that the return value will also be |
|
Its false on failure, but also false when false is the cached value... $success is the way to check for cache hit or miss. |
|
👍 ah of course Status: Reviewed |
| @@ -118,11 +118,13 @@ public function loadClass($class) | |||
| * | |||
| * @param string $class A class name to resolve to file | |||
| * | |||
| * @return string|null | |||
| * @return string|false | |||
There was a problem hiding this comment.
This does not seem correct. The other class loaders have string|null. As they are decorated by ApcClassLoader something is wrong. If they do not return false at all, then this whole change seems irrelevant.
|
Status: Needs Work |
bc3b4f5 to
90360c8
Compare
|
@Tobion fixed |
| $file = apcu_fetch($this->prefix.$class, $success); | ||
|
|
||
| if (!$success) { | ||
| apcu_store($this->prefix.$class, $file = $this->decorated->findFile($class) ?: null); |
There was a problem hiding this comment.
what is the point of the null condition?
There was a problem hiding this comment.
Composer's ClassLoader returns false here.
There was a problem hiding this comment.
then WinCache and Xcache class loaders should do the same normalization
|
|
90360c8 to
106ed06
Compare
|
@Tobion PR updated |
|
Thank you @nicolas-grekas. |
…nicolas-grekas) This PR was merged into the 2.3 branch. Discussion ---------- [ClassLoader] Fix storing not-found classes in APC cache | Q | A | ------------- | --- | Branch? | 2.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Commits ------- 106ed06 [ClassLoader] Fix storing not-found classes in APC cache