-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Use Sylius repositories instead of Doctrine in RouteProvider #2148
Use Sylius repositories instead of Doctrine in RouteProvider #2148
Conversation
@@ -51,7 +62,7 @@ public function getRouteByName($name, $parameters = array()) | |||
} | |||
} | |||
|
|||
$repositories = $this->getRepositories(); | |||
$repositories = $this->classRepositories; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to assign internal class variable to another variable as it's not being modified etc.
👍 |
…itory-service Use Sylius repositories instead of Doctrine in RouteProvider
Nice, thank you Gonzalo! 👍 |
NP! I needed it for the translations PR :-) |
@gonzalovilaseca Isn't this PR triggering a DB connection even when you clear the cache? Because I just updated Sylius and my CI server can't build it anymore since one of the last 10 commits forces a DB connection when you run I can confirm that this PR is the cause of the issue I have. Just tested it out. |
@steffenbrem Not that I'm aware of, I just injected repositories. Will have a look tomorrow just in case. |
@gonzalovilaseca I just uncommented the code that your PR provided. I can confirm that it is indeed triggering a DB connection when you clear your cache or you install your assets. This is ofcourse not a problem if you do manual deployments etc. But when you use a CI server there won't be a valid parameters.yml file to make the DB connection. It should just install the dependencies, assets (the whole post-update-cmd script from composer), nothing more. |
@steffenbrem Can you please provide instructions on how can I reproduce the problem without a CI server? |
@gonzalovilaseca Just make an invalid DB connection in your parameters.yml and then run clear cache command or run assets:install. This would throw an error that it could not connect to the database (which is obvious). Of course, running these kind of commands should not trigger a DB connection. |
@steffenbrem I believe the problem is not caused by my commit: I created a branch (git checkout -b test 0d6917c) without my commit and I'm getting the error you mention. |
@gonzalovilaseca Hmm. I did not create a branch based of a commit previous to your PR, but I just commented your lines and the issue was gone. Sylius is kind of broken right now. Because you cannot run @pjedrzejewski Whats your opinion in this? |
@gonzalovilaseca I see, I did a clean install of Sylius and it runs fine. Strange, looks like your commit only causes a problem in my own project. This is the stacktrace I got when running
You can see here that the following line in return $this->services['sylius.repository.product'] = new \Sylius\Bundle\ResourceBundle\Doctrine\ORM\EntityRepository($this->get('doctrine.orm.default_entity_manager'), $this->get('doctrine.orm.default_entity_manager')->getClassMetadata('Mango\\Component\\Core\\Model\\Product')); Any idea why this could trigger an error in my project (that has a composer dependency on this sylius repo) and it is not triggered in latest Sylius? For some reason, the database platform is already known by Sylius. But with my project it is unkown and trying to fetch it over the connection. I guess that is what happening. Now I only need to try and find why Sylius does not make a DB connection when it calls So I have found the culprit. I was using the I have used Symfony |
This PR refers to this RFC: #2147
Basically the RouteProvider looks for entities in repository classes instantiated by doctrine, this PR injects the Sylius repositories.