Skip to content

Conversation

@mikocevar
Copy link
Contributor

@mikocevar mikocevar commented May 31, 2024

Q A
Branch? 5.4
Bug fix? no
New feature? no
Deprecations? no
Issues Resolves developer confusion as seen here #53651
License MIT

It makes no sense to include a class with a private constructor since it cant be instantiated unless that happens within a static method. Skipping such a class in not in my scope of solutions, since that would leave the developer in the dark, just like it happened in the mentioned issue.

No additional test failed when running phpunit and also added a new one.

Symfony already covers a class with private constructor, but the message only appears when manually registering the class.

I hope I did everything okay this time, please review. Thank you!

Co-authored-by: Fabien Potencier <fabien@potencier.org>
@nicolas-grekas
Copy link
Member

I'm sorry but I don't think this is appropriate.
E.g. one can build services with a private constructor but a factory configured to make Symfony able to instantiate it.

@mikocevar
Copy link
Contributor Author

@nicolas-grekas You are right!

I played around for a while, and found out that the __construct is not a valid method for a factory, but it can still be present when configuring factories for creating services and my proposed change would block a valid configuration.

On top of that, I failed to check if the class loaded is also an Abstract class, which can also have a constructor, but is not instantiable.

No need to be sorry though, I learned quite a lot trying to fix this. Perhaps the best solution to this problem is simply improving the error message as proposed by another contributor here: #54890

I'm closing the PR, thank you for your review!

@mikocevar mikocevar closed this Jun 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants