Use is_file() instead of Filesystem::exists() #614
Conversation
lsmith77
added
the
State: Reviewing
| @@ -63,7 +63,7 @@ public function resolve($path, $filter) | |||
| */ | |||
| public function isStored($path, $filter) | |||
| { | |||
| return $this->filesystem->exists($this->getFilePath($path, $filter)); | |||
| return is_file($this->getFilePath($path, $filter)); | |||
There was a problem hiding this comment.
(Hopping in here from Twitter! Hi Lukas!)
Wouldn't you want to call it as \is_file() instead (same for the sys_get_temp_dir() call in WebPathResolverTest.php) so that the call shaves off the resolve time instead of looking for:
\Liip\ImagineBundle\Imagine\Cache\Resolver\is_file()
\Liip\ImagineBundle\Imagine\Cache\is_file()
\Liip\ImagineBundle\Imagine\is_file()
\Liip\ImagineBundle\is_file()
\Liip\is_file()
\is_file()
Although I can't remember if the lookup attempts between \Liip\ImagineBundle\Imagine\Cache\Resolver\is_file() and \is_file() are still the case.
There was a problem hiding this comment.
I do not have a link handy but afaik this isn't relevant in terms of performance.
There was a problem hiding this comment.
Indeed, although I would consider it good practice all-in-all (:
Use is_file() instead of Filesystem::exists()
lsmith77
removed
the
State: Reviewing
|
👍 |
Filesystem::exists()will return true no matter what, even if the file is actually a directory. This happens e.g. if you pass an empty path to the Twig filter. I know this breaks the Filesystem encapsulation but there is nois_file()equivalent in Filesystem.