Improve build step error reporting by allowing use of reflection in $target of exception

Author: markrandall

src/Framework/Build/BuildStepFailureException.php

@@ -3,8 +3,12 @@
 namespace phpweb\Framework\Build;
 
 use Exception;
+use Reflector;
 
 class BuildStepFailureException extends Exception
 {
-
+    public function __construct(string $message, public readonly Reflector $target)
+    {
+        parent::__construct($message);
+    }
 }

src/Framework/Build/BuildTools.php

@@ -13,11 +13,14 @@
 use RecursiveDirectoryIterator;
 use RecursiveIteratorIterator;
 use ReflectionClass;
+use ReflectionFunction;
+use ReflectionMethod;
 use SplFileInfo;
 use Throwable;
 use function count;
 use function fwrite;
 use function is_array;
+use function sprintf;
 use function time;
 use function token_get_all;
 use function trim;
@@ -84,7 +87,28 @@ public static function executeBuild(?LoggerInterface $logger = null): int
                 /* this allows our cache to kick in */
                 BuildTools::updateLastModifiedTime(time());
             } catch (BuildStepFailureException $e) {
-                fwrite(STDERR, 'Error building ' . $classId . ': ' . $e->getMessage());
+                $logger->critical("Build step '$classId' failed with an exception! " . $e->getMessage());
+
+                fwrite(STDERR,  "--------------------------\n");
+                fwrite(STDERR, "Step:    $classId\n");
+                fwrite(STDERR, "Error:   {$e->getMessage()}\n");
+
+                $target = $e->target;
+                if ($target instanceof ReflectionMethod) {
+                    fwrite(
+                        STDERR,
+                        sprintf("Target:  %s%s%s\n",
+                            $target->getDeclaringClass()->getName(),
+                            ($target->isStatic() ? '::' : '->'),
+                            $target->getName(),
+                        ),
+                    );
+                } elseif ($target instanceof ReflectionClass) {
+                    fwrite(STDERR, "Target:  {$target->getName()}\n");
+                } elseif ($target instanceof ReflectionFunction) {
+                    fwrite(STDERR, "Target:  {$target->getName()}\n");
+                }
+
                 exit(1);
             } catch (Throwable $e) {
                 fwrite(STDERR, 'Unhandled exception building ' . $classId . ': ' . $e->getMessage() . "\n");

src/Framework/Routing/Routing.php

@@ -14,16 +14,17 @@
 use phpweb\ProjectGlobals;
 use ReflectionClass;
 use ReflectionException;
+use RuntimeException;
 use Stringable;
 use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\Routing\Exception\MethodNotAllowedException;
 use Symfony\Component\Routing\Exception\ResourceNotFoundException;
 use Symfony\Component\Routing\Matcher\CompiledUrlMatcher;
 use Symfony\Component\Routing\Matcher\Dumper\CompiledUrlMatcherDumper;
 use Symfony\Component\Routing\RequestContext;
 use Symfony\Component\Routing\Route;
 use Symfony\Component\Routing\RouteCollection;
-use function array_unique;
 use function class_exists;
 use function explode;
 use function http_build_query;
@@ -55,12 +56,12 @@ class Routing implements BuildStep
     /**
      * Returns an array of the resolved route information, otherwise throws
      *
-     * @throws ResourceNotFoundException
-     * @throws MethodNotAllowedException
-     *
      * @return null|array{
      *    _controller: array{class-string, string}
      * }
+     * @throws MethodNotAllowedException
+     *
+     * @throws ResourceNotFoundException
      */
     public function match(Request $request): ?array
     {
@@ -72,7 +73,7 @@ public function match(Request $request): ?array
             $requestContext,
         );
 
-        /** @phpstan-ignore return.type  */
+        /** @phpstan-ignore return.type */
         return $matcher->matchRequest($request);
     }
 
@@ -86,12 +87,33 @@ public static function build(BuildContext $context): void
                 continue;
             }
 
+            if (!$class->isInstantiable()) {
+                throw new BuildStepFailureException(
+                    message: 'Controller #[Route]s cannot be placed on non-instantiable classes',
+                    target: $class,
+                );
+            }
+
             foreach ($class->getMethods() as $method) {
                 $methodAttr = ReflectionHelpers::getAttribute($method, RouteAttribute::class);
                 if (!$methodAttr) {
                     continue;
                 }
 
+                /* type checking */
+                $type = (string)$method->getReturnType();
+                if ($type !== Response::class && !is_subclass_of($type, Response::class)) {
+                    throw new BuildStepFailureException('Controller routes must return a Response', target: $method);
+                }
+
+                $methods = $methodAttr->methods ?: $classAttr->methods;
+                if (!$methods) {
+                    throw new BuildStepFailureException(
+                        message: 'Controller #[Route] must define at least one method at either class or method level',
+                        target: $method,
+                    );
+                }
+
                 /* titles get concatenated */
                 $path = ($classAttr->path ?? '');
                 if (is_string($methodAttr->path)) {
@@ -101,20 +123,27 @@ public static function build(BuildContext $context): void
                 /* to make our lives easier, we can replace the substitution tokens in the paths with
                  * symfony requires */
                 $replacements = $requirements = [];
-                preg_match_all(self::PATTERN_MATCH, $path, $matches, PREG_SET_ORDER);
-                foreach ($matches as $match) {
-                    [$full, $inner] = $match;
-
-                    [$name, $pattern] = self::parsePlaceholder($inner);
-                    $requirements[$name] = $pattern;
-                    $replacements[$full] = '{' . $name . '}';
+                try {
+                    preg_match_all(self::PATTERN_MATCH, $path, $matches, PREG_SET_ORDER);
+                    foreach ($matches as $match) {
+                        [$full, $inner] = $match;
+                        [$name, $pattern] = self::parsePlaceholder($inner);
+                        $requirements[$name] = $pattern;
+                        $replacements[$full] = '{' . $name . '}';
+                    }
+                } catch (\RuntimeException $e) {
+                    throw new BuildStepFailureException(
+                        message: "Controller #[Route] parsing error for '$path'; {$e->getMessage()}",
+                        target: $method,
+                    );
                 }
 
                 $normalizedPath = str_replace(array_keys($replacements), $replacements, $path);
-                $methods = array_values(array_unique($methodAttr->methods ?: $classAttr->methods ?: ['GET']));
                 $methodString = implode('|', $methods);
 
-                $context->logger->debug("Routing ($methodString) '$normalizedPath' to {$class->getName()}->{$method->getName()}");
+                $context->logger->debug(
+                    "Routing ($methodString) '$normalizedPath' to {$class->getName()}->{$method->getName()}",
+                );
 
                 $routes->add(
                     $normalizedPath,
@@ -212,14 +241,15 @@ public static function resolve(array|string $controller, array $params = [], arr
 
     /**
      * @return array{string,string}
+     * @throws \RuntimeException
      */
     private static function parsePlaceholder(string $input): array
     {
         $withoutCurley = substr($input, 1, strlen($input) - 2);
         [$name, $type] = explode(':', $withoutCurley);
 
         $matchPattern = self::PATTERNS[$type]
-            ?? throw new BuildStepFailureException("Unknown routing pattern '$type' for placeholder '$name'");
+            ?? throw new RuntimeException("Unknown routing pattern '$type' for placeholder '$name'");
 
         return [$name, $matchPattern];
     }