Skip to content

9.0: TASK: Simplify command handler resolution #3876

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

Merged
merged 3 commits into from
Sep 2, 2022

Conversation

bwaidelich
Copy link
Member

Replaces the explicit if/elseif blocks by simple checks
for a corresponding handle* method

Replaces the explicit `if/elseif` blocks by simple checks
for a corresponding `handle*` method
@skurfuerst
Copy link
Member

skurfuerst commented Sep 1, 2022

@bwaidelich I personally find the explicit if/else more explicit, so that would be my slight preference; but I'm also totally fine merging this change :) whatever you prefer!

(Especially because then, PHPStorm/PHPStan won't detect the method calls as uncalled; but can statically typecheck them - which won't work with this magic, or am I wrong?)

@bwaidelich
Copy link
Member Author

@skurfuerst I turned all the handle* methods private now (some weren't yet) and added some @noinspection PhpUnusedPrivateMethodInspection annotations to satisfy the IDE.
But I get your point and, yes, it won't be statically type-checked like this.

Maybe there's a third option to get rid of the large amount of repetition like:
image

And IMO we should go for a single approach (not use if/else in one handler, and match in others).

What about this:

public function canHandle(CommandInterface $command): bool
{
    return method_exists($this, 'handle' . (new \ReflectionClass($command))->getShortName());
}

public function handle(CommandInterface $command, ContentRepository $contentRepository): EventsToPublish
{
        return match ($command::class) {
            SetNodeProperties::class => $this->handleSetNodeProperties($command, $contentRepository),
            // ...
        }
}

@bwaidelich bwaidelich changed the title TASK: Simplify command handler resolution 9.0: TASK: Simplify command handler resolution Sep 2, 2022
@bwaidelich
Copy link
Member Author

@skurfuerst I like that final version now, what do you think?

@skurfuerst skurfuerst merged commit 019aef0 into 9.0 Sep 2, 2022
@skurfuerst skurfuerst deleted the feature/simplify-command-handling branch September 2, 2022 10:01
@skurfuerst
Copy link
Member

really nice :) let's get this in!

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