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 AbstractAdmin::createNewInstance method #6883

Merged
merged 1 commit into from
Feb 21, 2021

Conversation

franmomu
Copy link
Member

@franmomu franmomu commented Feb 21, 2021

Subject

Ref: #6850 (comment)

In 4.0, if a user wants to override the way in which their objects are created, they will have to override ModelManagerInterface::getModelInstance for each class.

The code from ModelManagerInterface::getModelInstance() in our persistence bundles (which has been copied to Instantiator::instantiate()), is not persistence related.

So this PR adds a AdminInterface::createNewInstance() method which the user can override to use a factory, service or any other pattern to create their objects.

I am targeting this branch, because this is BC.

Changelog

### Added
- Added `AbstractAdmin::createNewInstance()` to allow the user to customize how to create their objects.
### Deprecated
- Deprecated `ModelManagerInterface::getModelInstance()` method in favor of `AbstractAdmin::createNewInstance()`.

@franmomu franmomu added the minor label Feb 21, 2021
@VincentLanglet
Copy link
Member

Is it needed to make it public ? Cant it be protected instead ?

And should we deprecate alterNewInstance in favor of createNewInstance then ?

@franmomu
Copy link
Member Author

franmomu commented Feb 21, 2021

Is it needed to make it public ? Cant it be protected instead ?

mmm not really, I'll change it.

use Symfony\Component\Form\Extension\Core\Type\TextType;

final class TranslatedAdmin extends AbstractAdmin
{
public function createNewInstance(): object
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public function createNewInstance(): object
protected function createNewInstance(): object

@@ -25,7 +25,7 @@

class FooAdmin extends AbstractAdmin
{
public function getNewInstance()
public function createNewInstance(): object
Copy link
Member

Choose a reason for hiding this comment

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

protected

@franmomu
Copy link
Member Author

And should we deprecate alterNewInstance in favor of createNewInstance then ?

I'm not sure about this one to be honest, since it's called after appendParentObject I guess it's different.

VincentLanglet
VincentLanglet previously approved these changes Feb 21, 2021
@VincentLanglet VincentLanglet requested a review from a team February 21, 2021 10:42
Copy link
Contributor

@dmaicher dmaicher left a comment

Choose a reason for hiding this comment

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

LGTM but the commit message (and PR title) are a bit misleading since we are not adding anything to AdminInterface

Instead of overriding ModelManagerInterface::getModelInstance to
custom construct their objects, now the user can do it from the
related Admin class.

The Instantiator class will be use in 4.0 and its code is copied
from the persistence bundles.
@franmomu franmomu changed the title Add AdminInterface::createNewInstance method Add AbstractAdmin::createNewInstance method Feb 21, 2021
@VincentLanglet VincentLanglet merged commit 934b414 into sonata-project:3.x Feb 21, 2021
@VincentLanglet
Copy link
Member

Thanks @franmomu

@franmomu franmomu deleted the instance branch February 21, 2021 19:26
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.

3 participants