Skip to content

Commit

Permalink
Scrutinize code!
Browse files Browse the repository at this point in the history
Run phpunit with testdox option
Give phpunit data sets a name (shown in test results).
Better names for phpunit test methods.
PHPStan on level 7 (highest).
Added scrutinizer.yml.
Use Spatie\Regex, which throws Exceptions (wasn't checking on failed regexp).
Added .gitattributes, so test code won't be included on production.
  • Loading branch information
jasny committed Sep 9, 2019
1 parent d060a31 commit bcb5177
Show file tree
Hide file tree
Showing 14 changed files with 159 additions and 97 deletions.
10 changes: 10 additions & 0 deletions .gitattributes
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/tests export-ignore
/.gitattributes export-ignore
/.gitignore export-ignore
/.scrutinizer.yml export-ignore
/.travis.yml export-ignore
/infection.json.dist export-ignore
/phpunit.xml.dist export-ignore
/phpcs.xml.dist export-ignore
/phpstan.neon export-ignore
/README.md export-ignore
24 changes: 24 additions & 0 deletions .scrutinizer.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
#language: php
checks:
php: true
filter:
excluded_paths:
- tests
build:
nodes:
analysis:
environment:
php: 7.2
postgresql: false
redis: false
mongodb: false
tests:
override:
- phpcs-run src
-
command: vendor/bin/phpstan analyze --error-format=checkstyle | sed '/^\s*$/d' > phpstan-checkstyle.xml
analysis:
file: phpstan-checkstyle.xml
format: 'general-checkstyle'
- php-scrutinizer-run

12 changes: 8 additions & 4 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,17 @@ install:
- composer install --prefer-source $COMPOSER_FLAGS
- wget https://scrutinizer-ci.com/ocular.phar -O "$HOME/ocular.phar"

before_script:
- test "$TRAVIS_ALLOW_FAILURE" == "true" || export PHPUNIT_FLAGS="--coverage-clover cache/logs/clover.xml"
before_script: |
if (php -m | grep -q -i xdebug); then
export PHPUNIT_FLAGS="--coverage-clover cache/logs/clover.xml"
else
export PHPUNIT_FLAGS="--no-coverage"
fi
script:
- vendor/bin/phpunit $PHPUNIT_FLAGS

after_script:
- test "$TRAVIS_ALLOW_FAILURE" == "true" || vendor/bin/infection --only-covered --no-progress --no-interaction --threads=4
- test "$TRAVIS_ALLOW_FAILURE" == "true" || php "$HOME/ocular.phar" code-coverage:upload --format=php-clover cache/logs/clover.xml
- test "$PHPUNIT_FLAGS" == "--no-coverage" || vendor/bin/infection --only-covered --no-progress --no-interaction --threads=4
- test "$PHPUNIT_FLAGS" == "--no-coverage" || php "$HOME/ocular.phar" code-coverage:upload --format=php-clover cache/logs/clover.xml

7 changes: 4 additions & 3 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@
"jasny/reflection-factory": "^1.1.1",
"psr/http-factory": "^1.0",
"psr/http-message": "^1.0",
"psr/http-server-middleware": "^1.0"
"psr/http-server-middleware": "^1.0",
"spatie/regex": "^1.4"
},
"require-dev": {
"ext-json": "*",
Expand All @@ -46,8 +47,8 @@
"scripts": {
"test": [
"phpstan analyse",
"phpunit",
"infection --min-msi=95 --min-covered-msi=95 --only-covered --no-progress --no-interaction --threads=4",
"phpunit --testdox --colors=always",
"infection --min-msi=95 --min-covered-msi=95 --only-covered --no-progress --no-interaction --threads=4 --ansi",
"phpcs -p src"
]
},
Expand Down
2 changes: 1 addition & 1 deletion phpstan.neon
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
parameters:
level: 4
level: 7
paths:
- src
includes:
Expand Down
32 changes: 18 additions & 14 deletions src/Generator.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@
namespace Jasny\SwitchRoute;

use Jasny\SwitchRoute\Generator\GenerateFunction;
use LogicException;
use RuntimeException;
use Spatie\Regex\Regex;

/**
* Generate a PHP script for HTTP routing.
Expand Down Expand Up @@ -38,7 +37,7 @@ public function __construct(callable $generate = null)
* @param string $file Filename to store the script.
* @param callable $getRoutes Callback to get an array with the routes.
* @param bool $overwrite Overwrite existing file.
* @throws RuntimeException if file could not be created.
* @throws \RuntimeException if file could not be created.
*/
public function generate(string $name, string $file, callable $getRoutes, bool $overwrite): void
{
Expand All @@ -51,7 +50,7 @@ public function generate(string $name, string $file, callable $getRoutes, bool $

$code = ($this->generateCode)($name, $routes, $structure);
if (!is_string($code)) {
throw new LogicException("Expected code as string, got " . gettype($code));
throw new \LogicException("Expected code as string, got " . gettype($code));
}

if (!is_dir(dirname($file))) {
Expand All @@ -62,7 +61,7 @@ public function generate(string $name, string $file, callable $getRoutes, bool $
}

/**
* Try a file system function and throw a RuntimeException on failure.
* Try a file system function and throw a \RuntimeException on failure.
*
* @param callable $fn
* @param mixed ...$args
Expand All @@ -82,7 +81,7 @@ protected function tryFs(callable $fn, ...$args)
}

if ($ret === false) {
throw new RuntimeException(error_get_last()['message']);
throw new \RuntimeException(error_get_last()['message'] ?? "Unknown error");
}

return $ret;
Expand All @@ -107,25 +106,28 @@ protected function scriptExists(string $file): bool
/**
* Create a structure with a leaf for each endpoint.
*
* @param array $routes
* @param iterable $routes
* @return array
* @throws InvalidRouteException
*/
protected function structureEndpoints(array $routes): array
protected function structureEndpoints(iterable $routes): array
{
$structure = [];

foreach ($routes as $key => $route) {
if ($key === 'default') {
$structure["\e"] = (new Endpoint(''))->withRoute('', $routes['default'], []);
$structure["\e"] = (new Endpoint(''))->withRoute('', $route, []);
continue;
}

if (!preg_match('~^\s*\w+(?:\|\w+)*\s+/\S*\s*$~', $key)) {
$match = Regex::match('~^\s*(?P<methods>\w+(?:\|\w+)*)\s+(?P<path>/\S*)\s*$~', $key);

if (!is_string($key) || !$match->hasMatch()) {
throw new InvalidRouteException("Invalid routing key '$key': should be 'METHOD /path'");
}

[$methods, $varPath] = preg_split('~\s++~', trim($key));
[$segments, $vars] = $this->splitPath($varPath);
$methods = $match->namedGroup('methods');
[$segments, $vars] = $this->splitPath($match->namedGroup('path'));

$pointer =& $structure;
foreach ($segments as $segment) {
Expand Down Expand Up @@ -161,8 +163,10 @@ protected function splitPath(string $path): array
$vars = [];

foreach ($segments as $index => &$segment) {
if (preg_match('/^(?|:(?P<var>\w+)|\{(?P<var>\w+)\})$/', $segment, $match)) {
$vars[$match['var']] = $index;
$match = Regex::match('/^(?|:(?P<var>\w+)|\{(?P<var>\w+)\})$/', $segment);

if ($match->hasMatch()) {
$vars[$match->namedGroup('var')] = $index;
$segment = '*';
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/Generator/AbstractGenerate.php
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ abstract protected function generateRoute(string $key, array $route, array $vars
/**
* Generate the PHP script with a switch for routing.
*
* @param array $structure
* @param int $level Structure depth
* @param array<string,Endpoint|array> $structure
* @param int $level Structure depth
* @return string
*/
protected function generateSwitch(array $structure, int $level = 0): string
Expand Down
44 changes: 24 additions & 20 deletions src/Invoker.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,14 @@

namespace Jasny\SwitchRoute;

use BadMethodCallException;
use Closure;
use Jasny\ReflectionFactory\ReflectionFactory;
use Jasny\ReflectionFactory\ReflectionFactoryInterface;
use LogicException;
use ReflectionException;
use ReflectionFunction;
use ReflectionFunctionAbstract;
use ReflectionMethod;
use ReflectionNamedType;
use Spatie\Regex\Regex;

/**
* Invoke the action or script specified by the route.
Expand All @@ -39,7 +37,7 @@ class Invoker implements InvokerInterface
*/
public function __construct(?callable $createInvokable = null, ?ReflectionFactoryInterface $reflection = null)
{
$this->createInvokable = $createInvokable ?? Closure::fromCallable([__CLASS__, 'createInvokable']);
$this->createInvokable = $createInvokable ?? \Closure::fromCallable([__CLASS__, 'createInvokable']);
$this->reflection = $reflection ?? new ReflectionFactory();
}

Expand All @@ -62,30 +60,32 @@ public function generateInvocation(array $route, callable $genArg, string $new =
$invokable = ($this->createInvokable)($controller, $action);
$this->assertInvokable($invokable);

if (is_string($invokable) && strpos($invokable, '::') !== false) {
$invokable = explode('::', $invokable);
}

$reflection = $this->getReflection($invokable);
$call = $reflection instanceof ReflectionFunction
? $invokable
: $this->generateInvocationMethod($invokable, $reflection, $new);

return (is_string($invokable) ? $invokable : $this->generateInvocationMethod($invokable, $reflection, $new))
. '(' . $this->generateInvocationArgs($reflection, $genArg) . ')';
return $call . '(' . $this->generateInvocationArgs($reflection, $genArg) . ')';
}

/**
* Generate the code for a method call.
*
* @param callable&array $invokable
* @param ReflectionMethod $reflection
* @param string $new PHP code to instantiate class.
* @param array|string $invokable
* @param ReflectionMethod $reflection
* @param string $new PHP code to instantiate class.
* @return string
*/
protected function generateInvocationMethod(
array $invokable,
$invokable,
ReflectionMethod $reflection,
string $new = '(new \\%s)'
): string {
return $invokable[1] === '__invoke'
if (is_string($invokable) && strpos($invokable, '::') !== false) {
$invokable = explode('::', $invokable) + ['', ''];
}

return $invokable[1] === '__invoke' || $invokable[1] === ''
? sprintf($new, $invokable[0])
: ($reflection->isStatic() ? "{$invokable[0]}::" : sprintf($new, $invokable[0]) . "->") . $invokable[1];
}
Expand Down Expand Up @@ -120,11 +120,11 @@ protected function assertInvokable($invokable): void
{
$valid = is_callable($invokable, true) && (
(
is_string($invokable) && preg_match('/^[a-z_]\w*(\\\\\w+)*(::[a-z_]\w*)?$/i', $invokable)
is_string($invokable) && Regex::match('/^[a-z_]\w*(\\\\\w+)*(::[a-z_]\w*)?$/i', $invokable)->hasMatch()
) || (
is_array($invokable) &&
is_string($invokable[0]) && preg_match('/^[a-z_]\w*(\\\\\w+)*$/i', $invokable[0]) &&
preg_match('/^[a-z_]\w*$/i', $invokable[1])
is_string($invokable[0]) && Regex::match('/^[a-z_]\w*(\\\\\w+)*$/i', $invokable[0])->hasMatch() &&
Regex::match('/^[a-z_]\w*$/i', $invokable[1])->hasMatch()
)
);

Expand All @@ -141,7 +141,7 @@ protected function assertInvokable($invokable): void
$type = is_object($invokable) ? get_class($invokable) : gettype($invokable);
}

throw new LogicException("Invokable should be a function or array with class name and method, {$type} given");
throw new \LogicException("Invokable should be a function or array with class name and method, {$type} given");
}

/**
Expand All @@ -153,6 +153,10 @@ protected function assertInvokable($invokable): void
*/
protected function getReflection($invokable): ReflectionFunctionAbstract
{
if (is_string($invokable) && strpos($invokable, '::') !== false) {
$invokable = explode('::', $invokable);
}

return is_array($invokable)
? $this->reflection->reflectMethod($invokable[0], $invokable[1])
: $this->reflection->reflectFunction($invokable);
Expand Down Expand Up @@ -187,7 +191,7 @@ public function generateDefault(): string
final public static function createInvokable(?string $controller, ?string $action): array
{
if ($controller === null && $action === null) {
throw new BadMethodCallException("Neither controller or action is set");
throw new \BadMethodCallException("Neither controller or action is set");
}

[$class, $method] = $controller !== null
Expand Down
36 changes: 18 additions & 18 deletions tests/functional/TestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -48,30 +48,30 @@ public static function getRoutes(): array
public function provider()
{
return [
['GET', '/', "Some information"],
'GET /' => ['GET', '/', "Some information"],

['GET', '/users', array_values($this->users)],
['POST', '/users', "added user"],
['GET', '/users/1', $this->users[1]],
['GET', '/users/2', $this->users[2]],
['POST', '/users/1', "updated user '1'"],
['PUT', '/users/1', "updated user '1'"],
['DELETE', '/users/2', "deleted user '2'"],
'GET /users' => ['GET', '/users', array_values($this->users)],
'POST /users' => ['POST', '/users', "added user"],
'GET /users/1' => ['GET', '/users/1', $this->users[1]],
'GET /users/2' => ['GET', '/users/2', $this->users[2]],
'POST /users/1' => ['POST', '/users/1', "updated user '1'"],
'PUT /users/1' => ['PUT', '/users/1', "updated user '1'"],
'DELETE /users/1' => ['DELETE', '/users/2', "deleted user '2'"],

['GET', '/users/1/photos', $this->photos[1]],
['GET', '/users/2/photos', $this->photos[2]],
['POST', '/users/1/photos', "added photos for user 1"],
'GET /users/1/photos' => ['GET', '/users/1/photos', $this->photos[1]],
'GET /users/2/photos' => ['GET', '/users/2/photos', $this->photos[2]],
'POST /users/1/photos' => ['POST', '/users/1/photos', "added photos for user 1"],

['POST', '/export', ['data', 'export']],
'POST /export' => ['POST', '/export', ['data', 'export']],

['POST', '/foo', "404 Not Found", 404],
['DELETE', '/users', "405 Method Not Allowed (GET, POST)", 405],
['PATCH', '/users/1', "405 Method Not Allowed (GET, POST, PUT, DELETE)", 405],
'POST /foo (404)' => ['POST', '/foo', "404 Not Found", 404],
'DELETE /users (405)' => ['DELETE', '/users', "405 Method Not Allowed (GET, POST)", 405],
'PATCH /users/1 (405)' => ['PATCH', '/users/1', "405 Method Not Allowed (GET, POST, PUT, DELETE)", 405],

// Test with trailing slash
['GET', '/users/', array_values($this->users)],
['GET', '/users/2/', $this->users[2]],
['GET', '/users/2/photos/', $this->photos[2]],
'GET /users/' => ['GET', '/users/', array_values($this->users)],
'GET /users/2/' => ['GET', '/users/2/', $this->users[2]],
'GET /users/2/photos/' => ['GET', '/users/2/photos/', $this->photos[2]],
];
}
}
6 changes: 3 additions & 3 deletions tests/unit/EndpointTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@ public function testGetPath()
public function methodProvider()
{
return [
['GET', 'POST', 'PATCH'],
['get', 'post', 'patch'],
['Get', 'Post', 'Patch'],
'upper case' => ['GET', 'POST', 'PATCH'],
'lower case' => ['get', 'post', 'patch'],
'mixed case' => ['Get', 'Post', 'Patch'],
];
}

Expand Down
Loading

0 comments on commit bcb5177

Please sign in to comment.