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

[FEATURE] Migrate ObjectManager->get() to GeneralUtility::makeInstance() #1970

Merged
merged 1 commit into from
Feb 19, 2021
Merged

[FEATURE] Migrate ObjectManager->get() to GeneralUtility::makeInstance() #1970

merged 1 commit into from
Feb 19, 2021

Conversation

DanielSiepmann
Copy link
Contributor

@DanielSiepmann DanielSiepmann commented Feb 15, 2021

This migration assumes developers already migrated to dependency
injection where possible and useful.
All existing calls will be migrated.
In order to have proper working code, a Services.yaml is necessary.

Closes: #1883

This migration assumes developers already migrated to dependency
injection where possible and useful.
All existing calls will be migrated.
In order to have proper working code, a Services.yaml is necessary.

Relates: #1883
Copy link
Owner

@sabbelasichon sabbelasichon left a comment

Choose a reason for hiding this comment

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

i would not add this rule in the v10 set. This is confusing and many people do not know how to react. Should be part of the experimental ones or somewhere else.

@DanielSiepmann
Copy link
Contributor Author

Sure, would be fine for me.

There is only one experimental rector yet, and this is a concrete rector.
This one will be configuration. Should I only add the configuration somewhere?
Or should we add some kind of experimental set / configuration? Even those experimental changes are for specific TYPO3 versions.

@sabbelasichon
Copy link
Owner

@DanielSiepmann Good questions. I am not sure. Tiny change big impact. I would just add a experimental config at the beginning. Let´s see how far we will get. In the end, everyone can have a look at this configuration and select the desired rules. What do you think?

@MK-42
Copy link
Contributor

MK-42 commented Feb 17, 2021

What I see a lot in real life projects is the legacy instantiation of extbase domain objects with object manager. These could be migrated without breaking most projects with the proposed replacement of the get() call.

Maybe that could be added to the normal sets, and the changes that require a services.yaml could be incorporated in an experimental set?

I haven't looked into the whole new DI concept in v10, maybe we can provide some default services.yaml or extend an existing one along with this rector? Is it feasible to write such a config with a rector?

@MK-42
Copy link
Contributor

MK-42 commented Feb 17, 2021

On a second thought: objectManager->get for domain models is mostly used to be able to plug different implementations into the extbase context, or extending domain objects via inheritance.

For this to work there is the extbase containers ->Register ImplementationClassnallName, which must now be migrated to am XCLASS registration (if used). Maybe we could provide this as well? What do You think about this approach? This would of course only make sense for domain models.

@sabbelasichon
Copy link
Owner

@MK-42 @DanielSiepmann Do you know what´s the proposed solution to achieve the old behaviour of initializeObject and injections of service into entities (i know it is a code smell, but it is possible).

$containerConfigurator->import(__DIR__ . '/../../../../../../config/services.php');

$services = $containerConfigurator->services();
$services->set('typo3_objectmanagerget_to_generalutilitymakeinstance')
Copy link
Owner

Choose a reason for hiding this comment

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

I am nitpicking here, but i would prefer typo3_objectmanager_get_to_generalutility_makeinstance

@sabbelasichon sabbelasichon merged commit a664862 into sabbelasichon:master Feb 19, 2021
@DanielSiepmann DanielSiepmann deleted the feature/1883-objectmanager-get-to-makeinstance branch February 22, 2021 08:41
@DanielSiepmann
Copy link
Contributor Author

@MK-42 @DanielSiepmann Do you know what´s the proposed solution to achieve the old behaviour of initializeObject and injections of service into entities (i know it is a code smell, but it is possible).

No, and I would guess there is none. As far as I know Alexander is still more or less alone on Extbase front and tries to remove unnecessary stuff. Speaking of DI and makeInstance, I would guess one should better initialize within the constructor.

@MK-42
Copy link
Contributor

MK-42 commented Feb 22, 2021

@MK-42 @DanielSiepmann Do you know what´s the proposed solution to achieve the old behaviour of initializeObject and injections of service into entities (i know it is a code smell, but it is possible).

I think using injections via methods or property injection is an antipattern and there is no migration path for that.
Maybe we could introduce a phpstan error via saschas phpstan package. This would at least show an error for people who use injections in projects.
As for initializeObject I think that the framework should call that method still. But I can't find a prove for that at the moment.

The only thing rector could do imho would be to refactor that to GeneralUtility::makeInstance and objectManager->get. But that would not really improve the code, I think.

@simonschaufi simonschaufi linked an issue Oct 30, 2024 that may be closed by this pull request
simonschaufi added a commit that referenced this pull request Oct 30, 2024
simonschaufi added a commit that referenced this pull request Oct 30, 2024
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.

Deprecation: #90803 - ObjectManager::get in Extbase context
3 participants