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

Add generics to ModelManagerInterface #6721

Closed
wants to merge 1 commit into from

Conversation

franmomu
Copy link
Member

Subject

This should fix #6713 (comment), I think the problem was that given:

/**
 * @phpstan-return T
 */
public function getNewInstance()
{
    $object = $this->getModelManager()->getModelInstance($this->getClass());

    $this->appendParentObject($object);

    foreach ($this->getExtensions() as $extension) {
        $extension->alterNewInstance($this, $object);
    }

    return $object;
}

This line

$object = $this->getModelManager()->getModelInstance($this->getClass());

was not returning the same T. Now with:

/**
 * @phpstan-return ModelManagerInterface<T>
 */
public function getModelManager()

should be fine.

I am targeting this branch, because these changes are BC.

Changelog

### Added
- Add generics to `ModelManagerInterface`.

@franmomu franmomu added the minor label Dec 21, 2020
@VincentLanglet
Copy link
Member

I have some trouble to understand how the issue (#6713 (comment)) would come from the ModelManager since commenting the line $this->appendParentObject($object); was fixing the issue. (And it works well on 3.x)

I'm not sure those generics should be added. We generally use one ModelManager for every admin, and we're passing the class to most of the methods, so the modelManager shouldn't be related to one class T.

Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

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

#6713 is solved.

And I don't think the modelManager should be restricted to one class, since every class-specific methods are taking a param $class.

@SonataCI
Copy link
Collaborator

SonataCI commented Jan 1, 2021

Could you please rebase your PR and fix merge conflicts?

Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

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

@franmomu
Copy link
Member Author

I don't think it will work
https://phpstan.org/r/c9e2bf6d-3426-4a3f-8a96-cf96b8e98719

it depends, if the ModelManager it is for Foo, should be ModelManagerInterface<Foo>, if it is used for more, then should not use <object>, be generic and the property should have: /** @var ModelManager<Foo> */

@VincentLanglet
Copy link
Member

I don't think it will work
https://phpstan.org/r/c9e2bf6d-3426-4a3f-8a96-cf96b8e98719

if it is used for more, then should not use <object>, be generic and the property should have: /** @var ModelManager<Foo> */

I don't see any benefit in this case.
Method with a class-string as parameter are already using a template.

The issue you were trying to solve is solved, it shows that

$object = $this->getModelManager()->getModelInstance($this->getClass());

works well without the generic.

By adding a generic to ModelManager, you will forbid this code

/**
 * @phpstan-implements AdminInterface<Foo>
 */
class FooAdmin {

    public function something()
    {
        $object = $this->getModelManager()->getModelInstance(Bar::class);
    }
}

But this code is valid. The ModelManager is not restricted to the class of the subject.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants