-
-
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
Repository clean up and TranslatableResourceRepository deletion #3854
Repository clean up and TranslatableResourceRepository deletion #3854
Conversation
32d9e32
to
f2bcb33
Compare
|
||
namespace Sylius\Component\Addressing\Repository; | ||
|
||
use Doctrine\ORM\NonUniqueResultException; |
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.
This definitely cannot be in component.
8c16ae0
to
b10a0df
Compare
{ | ||
return $this->createQueryBuilder('o') | ||
->where('o.name = :name') | ||
->setParameter('name', $name) |
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.
The name is not translatable and we don't need to join anything, so I think this method is redundant and we should use simply findOneBy(array('name' => $name))
.
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.
👍
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.
We should join zone members here.
48a5df5
to
25c7d1b
Compare
@@ -32,8 +39,13 @@ public function findRootNodes(); | |||
* @param string $permalink | |||
* | |||
* @return TaxonInterface |
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.
|null
7991bc3
to
e564680
Compare
5796164
to
8a44742
Compare
8a44742
to
46c6f64
Compare
@pjedrzejewski Build should be green, so it is good to merge. But we have to resolve #4410 as soon as possible. |
Repository clean up and TranslatableResourceRepository deletion
Good work Łukasz! I am glad we have this cleaned up finally... |
I've made some clean ups in repositories and introduced some new repository interfaces. Moreover, I have also removed TranslatableResourceRepository and translation fields from configuration. It will reduce amount of magic in our repositories and will result in explicit calls to them. I am aware that this is a huge BC break, but it is better to do such changes earlier than later ;)
Update
NOTE: In order to be as explicit as it is possible, I removed all magic methods and functions like getQueryBuilder. Repositories will be much more readable if we will call methods directly, where one has to declare every used join. Previous implementation was hard to maintain as some times we used some objects joined in a parent class of parent class of parent class of the class we used. This was misleading and generated enormous amount of bugs. I removed some spec also, cuz implementation of repository has been changed and it's logic is tested anyway in a behat scenarios.
Uodate2
NOTE: Contains changes from #4271