Skip to content

Commit be401c0

Browse files
Rrego6Rrego6
committed
MC-24057: Implement static dependency analysis for wildcard and API urls
- Refactored code as per CR suggestions
1 parent 732be76 commit be401c0

File tree

3 files changed

+62
-63
lines changed

3 files changed

+62
-63
lines changed

‎dev/tests/static/framework/Magento/TestFramework/Dependency/PhpRule.php

Lines changed: 44 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,13 @@
1111

1212
use Magento\Framework\App\Utility\Files;
1313
use Magento\Framework\Config\Reader\Filesystem as ConfigReader;
14+
use Magento\Framework\Exception\ConfigurationMismatchException;
1415
use Magento\Framework\Exception\LocalizedException;
1516
use Magento\Framework\UrlInterface;
1617
use Magento\TestFramework\Dependency\Reader\ClassScanner;
1718
use Magento\TestFramework\Dependency\Route\RouteMapper;
1819
use Magento\TestFramework\Exception\NoSuchActionException;
19-
use Magento\Test\Integrity\Dependency\Converter;
20-
use PHPUnit\Framework\Exception;
20+
use Magento\TestFramework\Inspection\Exception;
2121

2222
/**
2323
* Rule to check the dependencies between modules based on references, getUrl and layout blocks
@@ -308,30 +308,28 @@ private function isPluginDependency($dependent, $dependency)
308308
* @param string $file
309309
* @return array
310310
* @throws LocalizedException
311-
* @throws \Exception
312-
* @SuppressWarnings(PMD.CyclomaticComplexity)
313311
*/
314312
protected function _caseGetUrl(string $currentModule, string &$contents, string $file): array
315313
{
316314
$dependencies = [];
317-
$pattern = '#(\->|:)(?<source>getUrl\(([\'"])(?<path>[a-zA-Z0-9\-_*\/]+)\3)\s*[,)]#';
315+
$pattern = '#(\->|:)(?<source>getUrl\(([\'"])(?<path>[a-zA-Z0-9\-_*/]+)\3)\s*[,)]#';
318316
if (!preg_match_all($pattern, $contents, $matches, PREG_SET_ORDER)) {
319317
return $dependencies;
320318
}
321319
try {
322320
foreach ($matches as $item) {
323321
$path = $item['path'];
324-
$returnedDependencies = [];
322+
$modules = [];
325323
if (strpos($path, '*') !== false) {
326-
$returnedDependencies = $this->processWildcardUrl($path, $file);
324+
$modules = $this->processWildcardUrl($path, $file);
327325
} elseif (preg_match('#rest(?<service>/V1/.+)#i', $path, $apiMatch)) {
328-
$returnedDependencies = $this->processApiUrl($apiMatch['service']);
326+
$modules = $this->processApiUrl($apiMatch['service']);
329327
} else {
330-
$returnedDependencies = $this->processStandardUrl($path);
328+
$modules = $this->processStandardUrl($path);
331329
}
332-
if ($returnedDependencies && !in_array($currentModule, $returnedDependencies)) {
330+
if ($modules && !in_array($currentModule, $modules)) {
333331
$dependencies[] = [
334-
'modules' => $returnedDependencies,
332+
'modules' => $modules,
335333
'type' => RuleInterface::TYPE_HARD,
336334
'source' => $item['source'],
337335
];
@@ -352,55 +350,43 @@ protected function _caseGetUrl(string $currentModule, string &$contents, string
352350
* @param string $filePath
353351
* @return string[]
354352
* @throws NoSuchActionException
355-
* @SuppressWarnings(PMD.CyclomaticComplexity)
356353
*/
357354
private function processWildcardUrl(string $urlPath, string $filePath)
358355
{
359356
$filePath = strtolower($filePath);
360357
$urlRoutePieces = explode('/', $urlPath);
361358
$routeId = array_shift($urlRoutePieces);
362-
363359
//Skip route wildcard processing as this requires using the routeMapper
364360
if ('*' === $routeId) {
365361
return [];
366362
}
367-
$filePathInfo = pathinfo($filePath);
368-
$fileActionName = $filePathInfo['filename'];
369-
$filePathPieces = explode(DIRECTORY_SEPARATOR, $filePathInfo['dirname']);
370363

371364
/**
372365
* Only handle Controllers. ie: Ignore Blocks, Templates, and Models due to complexity in static resolution
373366
* of route
374367
*/
375-
if (in_array('block', $filePathPieces)
376-
|| in_array('model', $filePathPieces)
377-
|| $filePathInfo['extension'] === 'phtml'
378-
) {
368+
if (!preg_match(
369+
'#controller/(adminhtml/)?(?<controller_name>.+)/(?<action_name>\w+).php$#',
370+
$filePath,
371+
$fileParts
372+
)) {
379373
return [];
380374
}
381-
$fileControllerIndex = array_search('adminhtml', $filePathPieces, true);
382-
if ($fileControllerIndex === false) {
383-
$fileControllerIndex = array_search('controller', $filePathPieces, true);
384-
}
385375

386376
$controllerName = array_shift($urlRoutePieces);
387377
if ('*' === $controllerName) {
388-
$fileControllerName = implode("_", array_slice($filePathPieces, $fileControllerIndex + 1));
389-
$controllerName = $fileControllerName;
378+
$controllerName = str_replace('/', '_', $fileParts['controller_name']);
390379
}
391380

392381
if (empty($urlRoutePieces) || !$urlRoutePieces[0]) {
393-
return $this->routeMapper->getDependencyByRoutePath(
394-
$routeId,
395-
$controllerName,
396-
UrlInterface::DEFAULT_ACTION_NAME
397-
);
382+
$actionName = UrlInterface::DEFAULT_ACTION_NAME;
383+
} else {
384+
$actionName = array_shift($urlRoutePieces);
385+
if ('*' === $actionName) {
386+
$actionName = $fileParts['action_name'];
387+
}
398388
}
399389

400-
$actionName = array_shift($urlRoutePieces);
401-
if ('*' === $actionName) {
402-
$actionName = $fileActionName;
403-
}
404390
return $this->routeMapper->getDependencyByRoutePath(
405391
$routeId,
406392
$controllerName,
@@ -418,7 +404,7 @@ private function processWildcardUrl(string $urlPath, string $filePath)
418404
private function processStandardUrl(string $path)
419405
{
420406
$pattern = '#(?<route_id>[a-z0-9\-_]{3,})'
421-
. '\/?(?<controller_name>[a-z0-9\-_]+)?\/?(?<action_name>[a-z0-9\-_]+)?#i';
407+
. '(/(?<controller_name>[a-z0-9\-_]+))?(/(?<action_name>[a-z0-9\-_]+))?#i';
422408
if (!preg_match($pattern, $path, $match)) {
423409
throw new NoSuchActionException('Failed to parse standard url path: ' . $path);
424410
}
@@ -434,30 +420,37 @@ private function processStandardUrl(string $path)
434420
}
435421

436422
/**
437-
* Helper method to get module dependencies used by an API URL
438-
*
439-
* @param string $path
440-
* @return string[]
441-
*
442-
* @throws NoSuchActionException
423+
* Create regex patterns from service url paths
424+
* @return array
443425
*/
444-
private function processApiUrl(string $path): array
426+
private function getServiceMethodRegexps(): array
445427
{
446-
/**
447-
* Create regex patterns from service url paths
448-
*/
449428
if (!$this->serviceMethods) {
450429
$this->serviceMethods = [];
451430
$serviceRoutes = $this->configReader->read()['routes'];
452431
foreach ($serviceRoutes as $serviceRouteUrl => $methods) {
453432
$pattern = '#:\w+#';
454-
$replace = '\w';
433+
$replace = '\w+';
455434
$serviceRouteUrlRegex = preg_replace($pattern, $replace, $serviceRouteUrl);
456-
$serviceRouteUrlRegex = '#' . $serviceRouteUrlRegex . '#';
435+
$serviceRouteUrlRegex = '#^' . $serviceRouteUrlRegex . '$#';
457436
$this->serviceMethods[$serviceRouteUrlRegex] = $methods;
458437
}
459438
}
460-
foreach ($this->serviceMethods as $serviceRouteUrlRegex => $methods) {
439+
return $this->serviceMethods;
440+
}
441+
442+
/**
443+
* Helper method to get module dependencies used by an API URL
444+
*
445+
* @param string $path
446+
* @return string[]
447+
*
448+
* @throws NoSuchActionException
449+
* @throws Exception
450+
*/
451+
private function processApiUrl(string $path): array
452+
{
453+
foreach ($this->getServiceMethodRegexps() as $serviceRouteUrlRegex => $methods) {
461454
/**
462455
* Since we expect that every service method should be within the same module, we can use the class from
463456
* any method
@@ -467,10 +460,10 @@ private function processApiUrl(string $path): array
467460

468461
$className = $method['service']['class'];
469462
//get module from className
470-
if (preg_match('#(?<module>\w+[\\\]\w+).*#', $className, $match)) {
463+
if (preg_match('#^(?<module>\w+[\\\]\w+)#', $className, $match)) {
471464
return [$match['module']];
472465
}
473-
throw new Exception('Failed to parse class from className' . $className);
466+
throw new Exception('Failed to parse class from className: ' . $className);
474467
}
475468
}
476469
throw new NoSuchActionException('Failed to match service with url path: ' . $path);

‎dev/tests/static/framework/tests/unit/testsuite/Magento/TestFramework/Dependency/PhpRuleTest.php

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -281,7 +281,6 @@ public function getDependencyInfoDataCaseGetUrlDataProvider()
281281
]
282282
],
283283
],
284-
//Skip processing routeid wildcards due to complexity in resolution
285284
'getUrl from routeid wildcard in controller' => [
286285
'Magento\Catalog\Controller\ControllerName\SomeClass',
287286
'$this->getUrl("*/Invalid/*")',
@@ -292,6 +291,11 @@ public function getDependencyInfoDataCaseGetUrlDataProvider()
292291
'$this->getUrl("Catalog/*/View")',
293292
[]
294293
],
294+
'getUrl from wildcard url within ignored Block class' => [
295+
'Magento\Cms\Block\SomeClass',
296+
'$this->getUrl("Catalog/*/View")',
297+
[]
298+
],
295299
'getUrl from wildcard url for ControllerName' => [
296300
'Magento\Catalog\Controller\Category\IGNORE',
297301
'$this->getUrl("Catalog/*/View")',
@@ -420,7 +424,7 @@ private function makeWebApiConfigReaderMock()
420424
'method' => 'save'
421425
] ],
422426
],
423-
'V1/products/:sku/options' => ['GET' => ['service' => [
427+
'/V1/products/:sku/options' => ['GET' => ['service' => [
424428
'class' => 'Magento\Catalog\Api\ProductCustomOptionRepositoryInterface',
425429
'method' => 'getList'
426430
] ] ]

‎dev/tests/static/testsuite/Magento/Test/Integrity/Dependency/Converter.php

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,10 @@ class Converter implements \Magento\Framework\Config\ConverterInterface
1313
/**#@+
1414
* Array keys for config internal representation.
1515
*/
16-
private const KEY_SERVICE_CLASS = 'class';
17-
private const KEY_SERVICE_METHOD = 'method';
16+
private const KEY_URL = 'url';
17+
private const KEY_CLASS = 'class';
18+
private const KEY_METHOD = 'method';
19+
private const KEY_ROUTE = 'route';
1820
private const KEY_ROUTES = 'routes';
1921
private const KEY_SERVICE = 'service';
2022
/**#@-*/
@@ -26,25 +28,25 @@ public function convert($source)
2628
{
2729
$result = [];
2830
/** @var \DOMNodeList $routes */
29-
$routes = $source->getElementsByTagName('route');
31+
$routes = $source->getElementsByTagName(self::KEY_ROUTE);
3032
/** @var \DOMElement $route */
3133
foreach ($routes as $route) {
3234
if ($route->nodeType != XML_ELEMENT_NODE) {
3335
continue;
3436
}
3537
/** @var \DOMElement $service */
36-
$service = $route->getElementsByTagName('service')->item(0);
37-
$serviceClass = $service->attributes->getNamedItem('class')->nodeValue;
38-
$serviceMethod = $service->attributes->getNamedItem('method')->nodeValue;
39-
$url = trim($route->attributes->getNamedItem('url')->nodeValue);
38+
$service = $route->getElementsByTagName(self::KEY_SERVICE)->item(0);
39+
$serviceClass = $service->attributes->getNamedItem(self::KEY_CLASS)->nodeValue;
40+
$serviceMethod = $service->attributes->getNamedItem(self::KEY_METHOD)->nodeValue;
41+
$url = trim($route->attributes->getNamedItem(self::KEY_URL)->nodeValue);
4042

41-
$method = $route->attributes->getNamedItem('method')->nodeValue;
43+
$method = $route->attributes->getNamedItem(self::KEY_METHOD)->nodeValue;
4244

4345
// We could handle merging here by checking if the route already exists
4446
$result[self::KEY_ROUTES][$url][$method] = [
4547
self::KEY_SERVICE => [
46-
self::KEY_SERVICE_CLASS => $serviceClass,
47-
self::KEY_SERVICE_METHOD => $serviceMethod,
48+
self::KEY_CLASS => $serviceClass,
49+
self::KEY_SERVICE => $serviceMethod,
4850
],
4951
];
5052
}

0 commit comments

Comments
 (0)