Skip to content
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

Use "self" typehints in AggregateRoot #28

Closed
wants to merge 1 commit into from
Closed

Use "self" typehints in AggregateRoot #28

wants to merge 1 commit into from

Conversation

spaceemotion
Copy link
Contributor

@spaceemotion spaceemotion commented Oct 27, 2019

While IDEs like PHPStorm can infer custom method calls due to usage of
"$this", actual code completion seems to only be working when the
correct method name is being called, as no auto-completion hints
appear when referencing the base "AggregateRoot" type.

I wasn't sure what to include in the changelog since subclasses could change the return type and may have to be updated. Would this be a .1 or a .0.1 release then?

While IDEs like PHPStorm can infer custom method calls due to usage of
"$this", actual code completion seems to only be working when the
correct method name is being called, as no auto-completion hints
appear when referencing the base "AggregateRoot" type.
@freekmurze
Copy link
Member

Thank you for this. I'm going to leave it like it at this point.

@freekmurze freekmurze closed this Oct 31, 2019
@spaceemotion
Copy link
Contributor Author

spaceemotion commented Oct 31, 2019

Are you planning an alternative solution for this? Without this fix, the project won't go through phpstan/larastan due to invalid typing.

PHPStorm also gives me warnings as soon as I use custom methods on the Aggregator:

final class PaymentAggregate extends AggregateRoot
{
    public function deletePayment()
    {
        $this->recordThat(new PaymentDeleted());

        return $this;
    }
}

gives me
image

As a workaround, I am now extending a custom AggregateRoot with typehints added, but adding this to every project is a bit tedious. Plus, this only fixes the PHPStorm IDE error, PHPStan still prints error messages.

<?php

declare(strict_types=1);

namespace App\Aggregates;

use Spatie\EventSourcing\ShouldBeStored;
use Spatie\EventSourcing\AggregateRoot as Base;

abstract class AggregateRoot extends Base
{
    /**
     * @param  string  $uuid
     * @return Base|$this
     */
    public static function retrieve(string $uuid): Base
    {
        return parent::retrieve($uuid);
    }

    /**
     * @param  ShouldBeStored  $domainEvent
     * @return Base|$this
     */
    public function recordThat(ShouldBeStored $domainEvent): Base
    {
        return parent::recordThat($domainEvent);
    }

    /**
     * @return Base|$this
     */
    public function persist(): Base
    {
        return parent::persist();
    }
}

I can change this PR to not use self as return types but rather add PHPDoc blocks like described above. That way the compatibility should not be affected in any way.

From some further tests, this is the only way to not have PHPStan nag about the method not existing on the root class.

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