Skip to content

[Symfony] Set few default common service names for Symfony App Analyzer #1687

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.

Sign up for GitHub

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

Merged
TomasVotruba merged 1 commit into from
Jul 4, 2019
Merged

[Symfony] Set few default common service names for Symfony App Analyzer #1687

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion packages/Symfony/src/Bridge/DefaultAnalyzedSymfonyApplicationContainer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ final class DefaultAnalyzedSymfonyApplicationContainer implements AnalyzedApplic
*/
private $commonNamesToTypes = [
'doctrine' => 'Doctrine\Bundle\DoctrineBundle\Registry',
'doctrine.orm.entity_manager' => 'Doctrine\ORM\EntityManagerInterface',
'doctrine.orm.default_entity_manager' => 'Doctrine\ORM\EntityManagerInterface',
Comment thread
stloyd marked this conversation as resolved.
];

/**
Expand All @@ -35,14 +37,19 @@ final class DefaultAnalyzedSymfonyApplicationContainer implements AnalyzedApplic
*/
private $containerFactory;

/**
* @param array<string, string> $commonNamesToTypes
*/
public function __construct(
ParameterProvider $parameterProvider,
SymfonyKernelParameterGuard $symfonyKernelParameterGuard,
ContainerFactory $containerFactory
ContainerFactory $containerFactory,
array $commonNamesToTypes = []
) {
$this->parameterProvider = $parameterProvider;
$this->symfonyKernelParameterGuard = $symfonyKernelParameterGuard;
$this->containerFactory = $containerFactory;
$this->commonNamesToTypes = array_merge($this->commonNamesToTypes, $commonNamesToTypes);
Copy link
Copy Markdown
Member

@TomasVotruba TomasVotruba Jul 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

What is this needed for? Default names should not depend on custom user input, but on default Symfony naming.

The point is 0-configuration. So services are loaded from the App container itself and we don't bother user to basically copy-paste it to rector.yaml config :)

Copy link
Copy Markdown
Contributor Author

@stloyd stloyd Jul 4, 2019

Choose a reason for hiding this comment

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

Problem is for example that by default container says that: doctrine.orm.entity_manager is instance of Doctrine\Common\Persistence\ObjectManager, which in fact is not really true, cause it's instance of Doctrine\ORM\EntityManagerInterface (which is also used for autowire functionality).

This is just one example, if there is more edge cases (in big legacy like we have, we can occur them most likely), user is not able to have automatic fix for such cases and he/she will be forced to fix it manually.

With this you can have most common known cases for replacement, but also user is able to not wait for new release of Rector if he/she finds another edge case.

Copy link
Copy Markdown
Member

@TomasVotruba TomasVotruba Jul 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This is just one example, if there is more edge cases (in big legacy like we have, we can occur them most likely), user is not able to have automatic fix for such cases and he/she will be forced to fix it manually.

I see. It makes sense now :) 👍

I miss this description on PR. Could you add it next time, so I can read the code with context of your intention? It would speed up the review

}

public function getTypeForName(string $name): ?string
Expand Down