Skip to content
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

Merged
merged 65 commits into from
Jun 20, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
65 commits
Select commit Hold shift + click to select a range
43c898f
Updated the class scanner so it calls SplFileInfo::realpath() only on…
kschroeder Mar 21, 2017
f1e6e2d
Merge branch 'develop' of https://github.com/magento/magento2 into de…
kschroeder Mar 21, 2017
acc587f
Updated the class scanner to reduce cyclomatic complexity.
kschroeder Mar 21, 2017
b127afa
Stupid cyclomatic complexity tests for stupid missing function doc co…
kschroeder Mar 21, 2017
1f5de9d
Fixed some PHPDoc mixups
kschroeder Mar 22, 2017
60a289b
Updated the class scanner so it calls SplFileInfo::realpath() only on…
kschroeder Mar 21, 2017
06865dc
Updated the class scanner to reduce cyclomatic complexity.
kschroeder Mar 21, 2017
98828dc
Stupid cyclomatic complexity tests for stupid missing function doc co…
kschroeder Mar 21, 2017
0c86dcd
Fixed some PHPDoc mixups
kschroeder Mar 22, 2017
60be236
Added a FileClassScanner which uses the tokenizer to extract only the…
kschroeder Mar 23, 2017
e46b0a9
Merge branch 'develop' of https://github.com/magento/magento2 into de…
kschroeder Mar 23, 2017
3a80af3
Fixed an issue with the FileClassScanner not liking one case when the…
kschroeder Mar 23, 2017
4aa7ce0
This includes a cache for class results because the individual class …
kschroeder Mar 23, 2017
55a765f
Fixed some mess detection issues. Removed the DirectoryList dependen…
kschroeder Mar 24, 2017
63927ba
Fixed mess detection issues
kschroeder Mar 24, 2017
bf87fdb
Fixed mess detection issues
kschroeder Mar 24, 2017
dd3f4b5
Another run at MD and Cyc. Complexity.
kschroeder Mar 24, 2017
a8abd1b
Updated for complexity and an else expression Codacy didn't like
kschroeder Mar 24, 2017
c593ef6
Updated for a code style issue
kschroeder Mar 24, 2017
ac82998
Updated for code review responses, handle namespaces better, and hand…
kschroeder Mar 24, 2017
70647be
Updated copyright date
kschroeder Mar 24, 2017
212b301
Merged a condition
kschroeder Mar 24, 2017
b8703c4
Updated code and docs from code mess
kschroeder Mar 24, 2017
ea90720
Disabled CyclomaticComplexity check. This code is sometimes called m…
kschroeder Mar 24, 2017
5fc1ea5
Disabled CyclomaticComplexity check. This code is sometimes called m…
kschroeder Mar 24, 2017
dc61477
Updated some MD and code review recommendations
kschroeder Mar 27, 2017
b1ba446
Updated to follow @schmengler's recommendation
kschroeder Mar 27, 2017
7f9d3fe
Updated based on recommendations
kschroeder Mar 27, 2017
5df626c
Removed a line
kschroeder Mar 29, 2017
f2f6b0a
Updated the class scanner so it calls SplFileInfo::realpath() only on…
kschroeder Mar 21, 2017
bc74883
Updated the class scanner to reduce cyclomatic complexity.
kschroeder Mar 21, 2017
1bafd4a
Stupid cyclomatic complexity tests for stupid missing function doc co…
kschroeder Mar 21, 2017
be9b572
Fixed some PHPDoc mixups
kschroeder Mar 22, 2017
5ff09a7
Updated the class scanner so it calls SplFileInfo::realpath() only on…
kschroeder Mar 21, 2017
28cec24
Updated the class scanner to reduce cyclomatic complexity.
kschroeder Mar 21, 2017
4b96b47
Stupid cyclomatic complexity tests for stupid missing function doc co…
kschroeder Mar 21, 2017
f4c174f
Fixed some PHPDoc mixups
kschroeder Mar 22, 2017
3375612
Added a FileClassScanner which uses the tokenizer to extract only the…
kschroeder Mar 23, 2017
4e8d1d6
Fixed an issue with the FileClassScanner not liking one case when the…
kschroeder Mar 23, 2017
4ee5b06
This includes a cache for class results because the individual class …
kschroeder Mar 23, 2017
aac32a0
Fixed some mess detection issues. Removed the DirectoryList dependen…
kschroeder Mar 24, 2017
043a7d6
Fixed mess detection issues
kschroeder Mar 24, 2017
bb7f32a
Fixed mess detection issues
kschroeder Mar 24, 2017
4898384
Another run at MD and Cyc. Complexity.
kschroeder Mar 24, 2017
b8c142a
Updated for complexity and an else expression Codacy didn't like
kschroeder Mar 24, 2017
cdcb776
Updated for a code style issue
kschroeder Mar 24, 2017
09d5ff3
Updated for code review responses, handle namespaces better, and hand…
kschroeder Mar 24, 2017
f660cb2
Updated copyright date
kschroeder Mar 24, 2017
6fbb247
Merged a condition
kschroeder Mar 24, 2017
cf39bfd
Updated code and docs from code mess
kschroeder Mar 24, 2017
da39630
Disabled CyclomaticComplexity check. This code is sometimes called m…
kschroeder Mar 24, 2017
67f5346
Disabled CyclomaticComplexity check. This code is sometimes called m…
kschroeder Mar 24, 2017
727439a
Updated some MD and code review recommendations
kschroeder Mar 27, 2017
77d9d2e
Updated to follow @schmengler's recommendation
kschroeder Mar 27, 2017
04b7dda
Updated based on recommendations
kschroeder Mar 27, 2017
9a7711e
Removed a line
kschroeder Mar 29, 2017
351c542
Merge branch 'develop' of https://github.com/magento/magento2 into de…
kschroeder Mar 30, 2017
b026b35
Merge branch 'develop' of https://github.com/kschroeder/magento2 into…
kschroeder Mar 30, 2017
00a91db
Changed visibility for includeClasses()
kschroeder Apr 11, 2017
81d82be
Changed type check
kschroeder Apr 11, 2017
1f55c6b
Changed visibility
kschroeder Apr 11, 2017
978ae40
Changed some spaces
kschroeder Apr 11, 2017
d7e601b
Changed some spaces
kschroeder Apr 11, 2017
cc72590
Updated for MD
kschroeder Apr 17, 2017
8f043c1
Updated for MD
kschroeder Apr 19, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 64 additions & 16 deletions setup/src/Magento/Setup/Module/Di/Code/Reader/ClassesScanner.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -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);
Copy link
Contributor

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

}

/**
Expand All @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition is really confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

@orlangur orlangur Mar 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean it should have been if (!$realPath) ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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]));
}
Expand All @@ -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;
}
}
Expand Down
171 changes: 171 additions & 0 deletions setup/src/Magento/Setup/Module/Di/Code/Reader/FileClassScanner.php
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

magento constructors should contain just assigments, and no business logic allowed inside

Copy link
Contributor

Choose a reason for hiding this comment

The 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:

2.3.1. Constructor SHOULD throw an exception when validation of an argument has failed.

In the example for 2.3.1, an array of options is validated and an InvalidArgumentException is thrown if one of the objects in the array doesn't implement a particular interface.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we forbid SuppressWarning for Cyclomatic complexity in newly created code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I would like a little guidance on what you would prefer: reduce the individual method complexity or notate MD to ignore CC?

If reducing the method complexity has performance impact, I'd say go for optimized performance in this case and mark the class as ignored.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Magento\Setup\Module\Di\Code\Reader\FileClassScanner::__construct I'd suggest to go with \Magento\Framework\Exception\FileSystemException. It's used the same way in \Magento\Framework\Filesystem\Directory\Write::assertIsFile.

For Magento\Setup\Module\Di\Code\Reader\FileClassScanner::isBracedNamespace, I'm a bit undecided which of the existing exeptions is the best fit. Maybe \Magento\Framework\Exception\NotFoundException, at least for one of the two cases?

{

}
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/

Copy link
Contributor

Choose a reason for hiding this comment

The 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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why need realpath here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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()
Expand Down
Loading