-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reduce calls to SplFileInfo::realpath() in the Magento\Setup\Module\Di\Code\Reader\ClassesScanner class #8965
Changes from all commits
43c898f
f1e6e2d
acc587f
b127afa
1f5de9d
60a289b
06865dc
98828dc
0c86dcd
60be236
e46b0a9
3a80af3
4aa7ce0
55a765f
63927ba
bf87fdb
dd3f4b5
a8abd1b
c593ef6
ac82998
70647be
212b301
b8703c4
ea90720
5fc1ea5
dc61477
b1ba446
7f9d3fe
5df626c
f2f6b0a
bc74883
1bafd4a
be9b572
5ff09a7
28cec24
4b96b47
f4c174f
3375612
4e8d1d6
4ee5b06
aac32a0
043a7d6
bb7f32a
4898384
b8c142a
cdcb776
09d5ff3
f660cb2
6fbb247
cf39bfd
da39630
67f5346
727439a
77d9d2e
04b7dda
9a7711e
351c542
b026b35
00a91db
81d82be
1f55c6b
978ae40
d7e601b
cc72590
8f043c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,8 +5,9 @@ | |
*/ | ||
namespace Magento\Setup\Module\Di\Code\Reader; | ||
|
||
use Magento\Framework\App\Filesystem\DirectoryList; | ||
use Magento\Framework\App\ObjectManager; | ||
use Magento\Framework\Exception\FileSystemException; | ||
use Magento\Setup\Module\Di\Code\Reader\FileScanner; | ||
|
||
class ClassesScanner implements ClassesScannerInterface | ||
{ | ||
|
@@ -15,12 +16,27 @@ class ClassesScanner implements ClassesScannerInterface | |
*/ | ||
protected $excludePatterns = []; | ||
|
||
/** | ||
* @var array | ||
*/ | ||
private $fileResults = []; | ||
|
||
/** | ||
* @var string | ||
*/ | ||
private $generationDirectory; | ||
|
||
/** | ||
* @param array $excludePatterns | ||
* @param string $generationDirectory | ||
*/ | ||
public function __construct(array $excludePatterns = []) | ||
public function __construct(array $excludePatterns = [], DirectoryList $directoryList = null) | ||
{ | ||
$this->excludePatterns = $excludePatterns; | ||
if ($directoryList === null) { | ||
$directoryList = ObjectManager::getInstance()->get(DirectoryList::class); | ||
} | ||
$this->generationDirectory = $directoryList->getPath(DirectoryList::GENERATION); | ||
} | ||
|
||
/** | ||
|
@@ -44,7 +60,14 @@ public function addExcludePatterns(array $excludePatterns) | |
*/ | ||
public function getList($path) | ||
{ | ||
|
||
$realPath = realpath($path); | ||
$isGeneration = strpos($realPath, $this->generationDirectory) === 0; | ||
|
||
// Generation folders should not have their results cached since they may actually change during compile | ||
if (!$isGeneration && isset($this->fileResults[$realPath])) { | ||
return $this->fileResults[$realPath]; | ||
} | ||
if (!(bool)$realPath) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This condition is really confusing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It simply excludes the generation directory from the local result cache if it exists. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean it should have been There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, well, you'd have to ask the original author of the class. I didn't write that line. |
||
throw new FileSystemException(new \Magento\Framework\Phrase('Invalid path: %1', [$path])); | ||
} | ||
|
@@ -53,46 +76,71 @@ public function getList($path) | |
\RecursiveIteratorIterator::SELF_FIRST | ||
); | ||
|
||
$classes = $this->extract($recursiveIterator); | ||
if (!$isGeneration) { | ||
$this->fileResults[$realPath] = $classes; | ||
} | ||
return $classes; | ||
} | ||
|
||
/** | ||
* Extracts all the classes from the recursive iterator | ||
* | ||
* @param \RecursiveIteratorIterator $recursiveIterator | ||
* @return array | ||
*/ | ||
private function extract(\RecursiveIteratorIterator $recursiveIterator) | ||
{ | ||
$classes = []; | ||
foreach ($recursiveIterator as $fileItem) { | ||
/** @var $fileItem \SplFileInfo */ | ||
if ($fileItem->isDir() || $fileItem->getExtension() !== 'php' || $fileItem->getBasename()[0] == '.') { | ||
continue; | ||
} | ||
$fileItemPath = $fileItem->getRealPath(); | ||
foreach ($this->excludePatterns as $excludePatterns) { | ||
if ($this->isExclude($fileItem, $excludePatterns)) { | ||
if ($this->isExclude($fileItemPath, $excludePatterns)) { | ||
continue 2; | ||
} | ||
} | ||
$fileScanner = new FileScanner($fileItem->getRealPath()); | ||
$fileScanner = new FileClassScanner($fileItemPath); | ||
$classNames = $fileScanner->getClassNames(); | ||
foreach ($classNames as $className) { | ||
if (empty($className)) { | ||
continue; | ||
} | ||
if (!class_exists($className)) { | ||
require_once $fileItem->getRealPath(); | ||
} | ||
$classes[] = $className; | ||
} | ||
$this->includeClasses($classNames, $fileItemPath); | ||
$classes = array_merge($classes, $classNames); | ||
} | ||
return $classes; | ||
} | ||
|
||
/** | ||
* @param array $classNames | ||
* @param string $fileItemPath | ||
* @return bool Whether the clas is included or not | ||
*/ | ||
private function includeClasses(array $classNames, $fileItemPath) | ||
{ | ||
foreach ($classNames as $className) { | ||
if (!class_exists($className)) { | ||
require_once $fileItemPath; | ||
return true; | ||
} | ||
} | ||
return false; | ||
} | ||
|
||
/** | ||
* Find out if file should be excluded | ||
* | ||
* @param \SplFileInfo $fileItem | ||
* @param string $fileItemPath | ||
* @param string $patterns | ||
* @return bool | ||
*/ | ||
private function isExclude(\SplFileInfo $fileItem, $patterns) | ||
private function isExclude($fileItemPath, $patterns) | ||
{ | ||
if (!is_array($patterns)) { | ||
$patterns = (array)$patterns; | ||
} | ||
foreach ($patterns as $pattern) { | ||
if (preg_match($pattern, str_replace('\\', '/', $fileItem->getRealPath()))) { | ||
if (preg_match($pattern, str_replace('\\', '/', $fileItemPath))) { | ||
return true; | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,171 @@ | ||
<?php | ||
|
||
/** | ||
* Copyright © 2013-2017 Magento, Inc. All rights reserved. | ||
* See COPYING.txt for license details. | ||
*/ | ||
|
||
namespace Magento\Setup\Module\Di\Code\Reader; | ||
|
||
class FileClassScanner | ||
{ | ||
/** | ||
* The filename of the file to introspect | ||
* | ||
* @var string | ||
*/ | ||
private $filename; | ||
|
||
/** | ||
* The list of classes found in the file. | ||
* | ||
* @var bool | ||
*/ | ||
private $classNames = false; | ||
|
||
/** | ||
* @var array | ||
*/ | ||
private $tokens; | ||
|
||
/** | ||
* Constructor for the file class scanner. Requires the filename | ||
* | ||
* @param string $filename | ||
*/ | ||
public function __construct($filename) | ||
{ | ||
$filename = realpath($filename); | ||
if (!file_exists($filename) || !\is_file($filename)) { | ||
throw new InvalidFileException( | ||
sprintf( | ||
'The file "%s" does not exist or is not a file', | ||
$filename | ||
) | ||
); | ||
} | ||
$this->filename = $filename; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. magento constructors should contain just assigments, and no business logic allowed inside There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @maghamed I'd like to argue that this is a valid use case according to 2.3.1 of the Technical Guidelines:
In the example for 2.3.1, an array of options is validated and an In the given code the provided filename has to be an existing file so for me, this is a valid validation. Which alternative approach would you suggest? |
||
} | ||
|
||
/** | ||
* Retrieves the contents of a file. Mostly here for Mock injection | ||
* | ||
* @return string | ||
*/ | ||
public function getFileContents() | ||
{ | ||
return file_get_contents($this->filename); | ||
} | ||
|
||
/** | ||
* Extracts the fully qualified class name from a file. It only searches for the first match and stops looking | ||
* as soon as it enters the class definition itself. | ||
* | ||
* Warnings are suppressed for this method due to a micro-optimization that only really shows up when this logic | ||
* is called several millions of times, which can happen quite easily with even moderately sized codebases. | ||
* | ||
* @SuppressWarnings(PHPMD.CyclomaticComplexity) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we forbid SuppressWarning for Cyclomatic complexity in newly created code There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #8965 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@schmengler Performance and Complexity are two different metrics. And in this specific case these metrics do not correlate. Because high Cyclomatic complexity doesn't lead to high performance and vice versa. In Magento we have a rule, that any new code should not have SuppressWarnings for CouplingBetween objects and CyclomaticComplexity. Because it's from the beginning has a Technical Debt. High CyclomaticComplexity often says that some functionality has not been fully covered with automated testing. Because it's too hard to cover all the possible conditional logic and exit points for this method. Moreover such code is fragile and really hard to maintain in future. Some time ago we even actively used CRAP metrics for this. So, this method should be decomposed on some smaller once There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @maghamed See #8965 (comment) for the explanation by @kschroeder why there might be a performance impact when decomposing this into more methods. How can we proceed here? Should we try to get hard numbers on the difference in performance? |
||
* @SuppressWarnings(PHPMD.NPathComplexity) | ||
* @return array | ||
*/ | ||
private function extract() | ||
{ | ||
$allowedOpenBraces = [T_CURLY_OPEN, T_DOLLAR_OPEN_CURLY_BRACES, T_STRING_VARNAME]; | ||
$classes = []; | ||
$namespace = ''; | ||
$class = ''; | ||
$triggerClass = false; | ||
$triggerNamespace = false; | ||
$braceLevel = 0; | ||
$bracedNamespace = false; | ||
|
||
$this->tokens = token_get_all($this->getFileContents()); | ||
foreach ($this->tokens as $index => $token) { | ||
// Is either a literal brace or an interpolated brace with a variable | ||
if ($token == '{' || (is_array($token) && in_array($token[0], $allowedOpenBraces))) { | ||
$braceLevel++; | ||
} else if ($token == '}') { | ||
$braceLevel--; | ||
} | ||
// The namespace keyword was found in the last loop | ||
if ($triggerNamespace) { | ||
// A string ; or a discovered namespace that looks like "namespace name { }" | ||
if (!is_array($token) || ($namespace && $token[0] == T_WHITESPACE)) { | ||
$triggerNamespace = false; | ||
$namespace .= '\\'; | ||
continue; | ||
} | ||
$namespace .= $token[1]; | ||
|
||
// The class keyword was found in the last loop | ||
} else if ($triggerClass && $token[0] == T_STRING) { | ||
$triggerClass = false; | ||
$class = $token[1]; | ||
} | ||
|
||
switch ($token[0]) { | ||
case T_NAMESPACE: | ||
// Current loop contains the namespace keyword. Between this and the semicolon is the namespace | ||
$triggerNamespace = true; | ||
$namespace = ''; | ||
$bracedNamespace = $this->isBracedNamespace($index); | ||
break; | ||
case T_CLASS: | ||
// Current loop contains the class keyword. Next loop will have the class name itself. | ||
if ($braceLevel == 0 || ($bracedNamespace && $braceLevel == 1)) { | ||
$triggerClass = true; | ||
} | ||
break; | ||
} | ||
|
||
// We have a class name, let's concatenate and store it! | ||
if ($class != '') { | ||
$namespace = trim($namespace); | ||
$fqClassName = $namespace . trim($class); | ||
$classes[] = $fqClassName; | ||
$class = ''; | ||
} | ||
} | ||
return $classes; | ||
} | ||
|
||
/** | ||
* Looks forward from the current index to determine if the namespace is nested in {} or terminated with ; | ||
* | ||
* @param integer $index | ||
* @return bool | ||
*/ | ||
private function isBracedNamespace($index) | ||
{ | ||
$len = count($this->tokens); | ||
while ($index++ < $len) { | ||
if (!is_array($this->tokens[$index])) { | ||
if ($this->tokens[$index] == ';') { | ||
return false; | ||
} else if ($this->tokens[$index] == '{') { | ||
return true; | ||
} | ||
continue; | ||
} | ||
|
||
if (!in_array($this->tokens[$index][0], [T_WHITESPACE, T_STRING, T_NS_SEPARATOR])) { | ||
throw new InvalidFileException('Namespace not defined properly'); | ||
} | ||
} | ||
throw new InvalidFileException('Could not find namespace termination'); | ||
} | ||
|
||
/** | ||
* Retrieves the first class found in a class file. The return value is in an array format so it retains the | ||
* same usage as the FileScanner. | ||
* | ||
* @return array | ||
*/ | ||
public function getClassNames() | ||
{ | ||
if ($this->classNames === false) { | ||
$this->classNames = $this->extract(); | ||
} | ||
return $this->classNames; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
<?php | ||
|
||
namespace Magento\Setup\Module\Di\Code\Reader; | ||
|
||
class InvalidFileException extends \InvalidArgumentException | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Custom Exceptions are not recommended to be introduced in each module. Here is more discussion on this subject - https://github.com/magento/magento2/pull/9315/files#r112514206 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For For |
||
{ | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,16 +5,31 @@ | |
*/ | ||
namespace Magento\Setup\Test\Unit\Module\Di\Code\Reader; | ||
|
||
use Magento\Framework\App\Filesystem\DirectoryList; | ||
|
||
class ClassesScannerTest extends \PHPUnit_Framework_TestCase | ||
{ | ||
/** | ||
* @var \Magento\Setup\Module\Di\Code\Reader\ClassesScanner | ||
*/ | ||
private $model; | ||
|
||
/** | ||
* the /var/generation directory realpath | ||
* | ||
* @var string | ||
*/ | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. empty line |
||
private $generation; | ||
|
||
protected function setUp() | ||
{ | ||
$this->model = new \Magento\Setup\Module\Di\Code\Reader\ClassesScanner(); | ||
$this->generation = realpath(__DIR__ . '/../../_files/var/generation'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To get a clear indication that it is the path that is broken if there is an error during debugging. |
||
$mock = $this->getMockBuilder(DirectoryList::class)->disableOriginalConstructor()->setMethods( | ||
['getPath'] | ||
)->getMock(); | ||
$mock->method('getPath')->willReturn($this->generation); | ||
$this->model = new \Magento\Setup\Module\Di\Code\Reader\ClassesScanner([], $mock); | ||
} | ||
|
||
public function testGetList() | ||
|
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 allowed to make function calls in the constructor, just assignments to class variables.
here we receive $directoryList and after that make a call to get Path