Skip to content

Refactor Dispatcher to improve type hints and simplify callable handling - #721

Open
fadrian06 wants to merge 35 commits into
masterfrom
dispatcher-rework
Open

Refactor Dispatcher to improve type hints and simplify callable handling#721
fadrian06 wants to merge 35 commits into
masterfrom
dispatcher-rework

Conversation

@fadrian06

Copy link
Copy Markdown
Contributor

This pull request introduces a new utility for handling callables with before and after filters, and makes a minor update to the coding standards configuration. The most important changes are summarized below.

New Features

  • Added a new FilteredCallable class in flight/core/FilteredCallable.php that allows wrapping a callable with before and after filters, enabling pre- and post-processing of function input and output. This utility supports flexible extension and control over callable execution.

Code Quality and Standards

  • Updated phpcs.xml.dist to exclude the SpacingAfterOpenBrace rule from PSR2, allowing more flexibility in code formatting for control structures.

Type Annotations

  • Removed a redundant @phpstan-template annotation from the Engine.php docblock, likely as part of code cleanup or refactoring.

…(self explanatory).

- FilteredCallable wraps a callable and helps static analyzers to check if after filters are using the callable return type as output type.
- FilteredCallable handles after filters with two parameters (deprecated) and one parameter (new required signature)
Now filters are handled by each FilteredCallable.
Copilot AI lite review requested due to automatic review settings August 16, 2026 05:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors flight\core\Dispatcher to simplify callable execution and introduces a new FilteredCallable wrapper to run “before” and “after” filters around a callable, alongside minor static-analysis / coding-standards adjustments.

Changes:

  • Added flight\core\FilteredCallable to wrap a callable with before/after filters.
  • Refactored flight\core\Dispatcher to use FilteredCallable for named callables and updated container/callable handling.
  • Updated PHPCS ruleset (phpcs.xml.dist) and adjusted PHPStan docblocks in Engine.php / Dispatcher.php.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
phpcs.xml.dist Tunes PHPCS rules by excluding a specific PSR2 spacing sniff.
flight/Engine.php Docblock annotation cleanup (but currently leaves broken PHPStan generic references).
flight/core/FilteredCallable.php New callable wrapper that applies before/after filter chains.
flight/core/Dispatcher.php Dispatcher refactor to route named callables through FilteredCallable and simplify execution paths.
Suppressed comments (6)

flight/core/Dispatcher.php:252

  • hook() assumes get($name) returns a FilteredCallable and calls pushBeforeFilter()/pushAfterFilter(). However get() can return a non-object callable (e.g. a Closure from the deprecated $events map), which would cause a fatal error here. Guard with instanceof FilteredCallable (or wrap the callable).
        $filteredCallable = $this->get($name);

        if ($filteredCallable) {
            if ($type === self::FILTER_BEFORE) {
                $filteredCallable->pushBeforeFilter($callback);

flight/core/Dispatcher.php:300

  • execute() assigns $container = $this->containerHandler; but never uses it. This is dead code and can be removed to avoid confusion.
    public function execute($callback, array $params = [])
    {
        $container = $this->containerHandler;

        $this->verifyValidFunction($callback);

flight/core/Dispatcher.php:476

  • verifyValidClassCallable() only validates method existence when $class is an object. If $class is a class-string and the method doesn't exist, this will fall through and later cause a runtime Error when calling $class->$method(...). Validate method_exists($class, $method) for class-strings too and throw an InvalidArgumentException consistently.
        if (!is_object($class) && !class_exists($class)) {
            $message = "Class '$class' not found. Is it being correctly autoloaded with Flight::path()?";
            $exception = new InvalidArgumentException($message);
        } elseif ($this->containerException) {
            $exception = $this->containerException;

flight/core/Dispatcher.php:506

  • resolveContainerClass() only catches ContainerExceptionInterface. PSR-11 get() may also throw NotFoundExceptionInterface (which does not extend ContainerExceptionInterface), so those exceptions will currently escape and bypass the output-buffer fix logic. Catch Throwable here (as the previous implementation did) so all container failures are handled consistently.
        if ($container instanceof Container) {
            try {
                return $container->get($class);
            } catch (ContainerExceptionInterface $exception) {
                $this->containerException = $exception;

flight/core/FilteredCallable.php:84

  • When adapting 2-parameter after filters, the wrapper passes a static empty $input array to the original filter. This means after-filters that expect to inspect/modify the actual invocation args will silently receive [] instead of the real input.
                $filter = static function (&$output) use ($filter) {
                    static $input = [];

                    return $filter($input, $output);
                };

flight/core/Dispatcher.php:186

  • The get() docblock says it returns ?FilteredCallable, but the return type is ?callable and the method can also return other callables from the deprecated $events map. Update the docblock to match the actual return value.
    /**
     * Returns a callable by its name.
     *
     * @param string $name Callable name.
     * @return ?FilteredCallable
     */
    public function get(string $name): ?callable
    {
        return $this->namedCallables[$name] ?? $this->events[$name] ?? null;

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread flight/Engine.php
Comment thread flight/core/Dispatcher.php
Comment thread flight/core/FilteredCallable.php
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants