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

ModelManagerInterface and getParentMetadataForProperty() #1296

Closed
dmaicher opened this issue Feb 4, 2021 · 2 comments · Fixed by #1350
Closed

ModelManagerInterface and getParentMetadataForProperty() #1296

dmaicher opened this issue Feb 4, 2021 · 2 comments · Fixed by #1350

Comments

@dmaicher
Copy link
Contributor

dmaicher commented Feb 4, 2021

Feature Request

Inside the AbstractTypeGuesser we are calling Sonata\AdminBundle\Model\ModelManagerInterface::getParentMetadataForProperty but this method is not part of that contract.

So we are actually expecting a Sonata\DoctrineORMAdminBundle\Model\ModelManager for it to work.

As @VincentLanglet suggested we could

  • extend the ModelManagerInterface and add this method
  • move this method somewhere else

Or any other ideas?

@franmomu
Copy link
Member

franmomu commented Feb 4, 2021

I created sonata-project/SonataAdminBundle#6701 for this, but I haven't tried it yet.

@VincentLanglet
Copy link
Member

The getParentMetadataForProperty method was also used in the fixDescription method of our builders.
@dmaicher you removed it from FormContractor, ListBuilder and ShowBuilder, but you had issues with the tests for the DatagridBuilder. With https://github.com/sonata-project/SonataDoctrineORMAdminBundle/pull/1297/files, it will be definitely removed from the builders.

So indeed, we just have to focus about the TypeGuesser usage.

$this->guesser->guessType($admin->getClass(), $fieldDescription->getName(), $admin->getModelManager())

This could be easily migrated to

$this->guesser->guessType($fieldDescription)

Because as a first step, we can change the code inside guessType to add

$admin = $fieldDescription->getAdmin();
$class = $admin->getClass();
$modelManager = $admin->getModelManager();

And keep the same code.

Note that currently, when guessType is called, we did not attach the admin
It's done instead in the fixFieldDescription method:
https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/3.x/src/Builder/DatagridBuilder.php#L74
So there is something to change about this.
We can move the setAdmin call from fixFieldDescription to addFilter.

I also notice that the fixFieldDescription is in every builder, but is only called in the SonataAdmin::FormMapper https://github.com/sonata-project/SonataAdminBundle/search?q=fixFieldDescription ; there is no need to make it public in other builders.
A maybe better idea could be to move the addFilter and addField to the SonataAdmin code. When I look at the code I think it just require a BuilderInterface::getGuesser method and a DatagridBuilderInterface::getFilterFactory method.

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

Successfully merging a pull request may close this issue.

3 participants