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

separate public and private parts #4465

Closed
wants to merge 3 commits into from

Conversation

franzholz
Copy link
Contributor

The long text shall not mix the description for private and public classes.

The long text shall not mix the description for private and public classes.

**public**:

* Every class that is directly retrieved using :php:`GeneralUtility::makeInstance()` and
Copy link
Member

Choose a reason for hiding this comment

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

I dont think the nested list and pseudo headers improve readability...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How then can it be improved?

Copy link
Contributor

@sbuerk sbuerk Jul 5, 2024

Choose a reason for hiding this comment

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

First,the explanation for private and public is wrong.

The second thing is, that this relates to Dependency Injection.

To sum it up:

  • A class not requiring any autowiring (Constructor Injection, Factory values, injectMethod injection) and are instancable using plain new geword literally does not need anything of this as GU::makeInstance() is cabalbe of creating them using the new keyword.
  • A class which needs autowiring (injection of other services) by the Dependency Injection, but is itself not injected anyway and only retrieved with GU::makeInstance() needs to be marked public: true in the DI, to avoid that the Symfony DI container removes this class because no class needs injection of that class.
  • A class which needs autowriing by the DI and is at least injected in one class throughout active code AND additionally retrieved using GU::makeInstance() can stay public: false, which should be the default if you follow the suggested default extension DI snippet for a extension Services.yaml.

Note that this also counts if you want to retrieve a class directly from the DI container (which is internally done in GU::makeInstance() - thus not beeing related to GU::makeInstance() directly but to retrieving manually from the DI container. Different things.

I'd suggest to revise the DI documentation, to enhance the topic about that more slightly and only make a small hint here with a link to the DI section - where it belongs.

A class not needing autowiring, but are available in the DI container and needs to be injected will be injected even with public: false - and GU::makeInstance() cann fallback to a simple new ClassName() without any issue due not having to inject something. Only in case it is never injected and needs autowiring public: true is needed.

Beside that, with TYPO3 v13 the AutoWire PHP Attribute from Symfony can be used instead of configuring it in the Services.yaml/php - which the core already changed recently.

Copy link
Contributor

Choose a reason for hiding this comment

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

One additonal remark - for GU::makeInstance() it is required to avoid providing and constructor arguments to the GU::makeInstance call. Otherwise it will always be created using the new keyword - if registered public or non-public in DI or not.

@linawolf
Copy link
Member

I will close this ticket and move @sbuerk suggestions to a ticket.

@franzholz
Copy link
Contributor Author

franzholz commented Jul 11, 2024

Why do you close this so quickly? All the remarks from Stefan Bürk are very important for developers.
Please do not leave all those wrong texts in the documentation!

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.

None yet

3 participants