I think putting all of this new code into a private method in this class might be nice. Or, its own class, which could then be unit tested a bit easier - like UserClassBuilder

I added a new private method like you said first because i think this stuff is really specific to this command with a good test coverage. See if you like :)

I believe this is not needed anymore. And ClassSourceManipulator is just ignoring it because it's already there.

Done.

I just don't want this to have a default value... so let's leave it null.

And maybe we should call these properties:overriddenReturnType and isOverriddenReturnTypeNullable... because that's what they really are. If they are not set, then it means that the "default" return type of the related class would be used.

This needs to be reverted - your editor may have done this automatically

I think I may have confused you. Here is how I would expect the new parts of this class to look (I'm also changing the name "overridden" to "custom"):

private $customReturnType;
private $isCustomReturnTypeNullable;

public function getCustomReturnType(): ?string
{
    return $this->customReturnType;
}

public function isCustomReturnTypeNullable(): bool
{
    return $this->isCustomReturnTypeNullable;
}

public function setCustomReturnType(string $customReturnType, bool $isNullable): void
{
    $this->customReturnType = $customReturnType;
    $this->isCustomReturnTypeNullable = $isNullable;
}

Then, in ClassSourceManipulator, when we use this, the code would be:

        $this->addGetter(
            $relation->getPropertyName(),
            $typeHint,
            $relation->getCustomReturnType() ?: $typeHint,
            // getter methods always have nullable return values
            // unless this has been customized explicitly
            $relation->getCustomReturnType() ? $relation->isCustomReturnTypeNullable() : $typeHint,
            $relation->isOverriddenReturnTypeNullable()
        );

For now, without modifying other parts of the MakerBundle, i think we should do things like this in BaseRelation

    private $customReturnType;
    private $isCustomReturnTypeNullable = true;

    public function isCustomReturnTypeNullable(): bool
    {
        return $this->isCustomReturnTypeNullable;
    }


    public function setCustomReturnType(string $customReturnType, bool $isNullable)
    {
        $this->customReturnType = $customReturnType;
        $this->isCustomReturnTypeNullable = $isNullable;

        return $this;
    }

And this in ClassSourceManipulator

$this->addGetter(
            $relation->getPropertyName(),
            $relation->getCustomReturnType() ?: $typeHint,
            // getter methods always have nullable return values
            // unless this has been customized explicitly
            $relation->isCustomReturnTypeNullable()
        );

I think (?) this isn't needed. It's true that isNullable could be null, but the :bool return type will cast null into a false boolean.

Done.

What's the problem in PHP 7.3? We need to be explicit. And if there IS a problem currently in 7.3, then that's a block for the PR and we should be clear that this is still a "TODO" (like by posting a comment about it in the PR or in the PR description)

Not php 7.3, i wrote that because only the test for php 7.3 failed in travis.

jrushlow added new code. Our targetClass is generaated as a fetch class UserCustom::class not in FQCN "App\Entity\UserCustom"

As we talked about, if we want to avoid the comments in the repository class, let's pass an argument to EntityClassGenerator::generateEntityClass() to make it not generate the comments. That method name is getting a little long, but that's ok - it's an internal class.

Hopefully this won't be needed at the end :)

Yes :)