Sylius: [AdminBundle] Product isn't displaying if the translation for the current locale is missing
Sylius version affected: As far as I know - all versions Description
If the product has not defined translation for the current locale it isn’t displayed on the products list in AdminPanel.
Let’s say we have 2 active locales in our shop en_US and pl_PL. We have defined translations for the both locales for the all products. We want to add de_DE locale. We add de_DE locale in AdminPanel, hire the translator and set his AP language to Germany (cause he’s German 😄). He goes /admin/products and sees nothing. The product list is empty.
Steps to reproduce
The easiest way:
- Check your shop locales
- Change your AP language to anyone not defined in the locales list
- Go to
/admin/products
Possible Solution
The solutions is pretty simple. In Sylius\Bundle\CoreBundle\Doctrine\ORM\ProductRepository::createListQueryBuilder the translation is joined using innerJoin. The result is empty list if product title for the current locale is missing. I’ve managed to fix this by changing innerJoin to leftJoin.
The only problem I’ve found is the empty Name column in case of not defined title for the current locale. I’m considering the following solutions:
a) If there’s no title for the current locale just display ‘-’ - ugly but works, and still better than no products displayed
b) Add some text (?) informing there’s no translation for your locale - a lot of work with translating for all supported languages
c) Remove Name column (😄) - a little drastic way, but at the end of the day I think it would be the best solution. In my opinion it’s enough for a Shop Admin to have Product’s code to identify the right product.
I can prepare the PR fixing this problem (it’s almost done btw) but I would like to ask you to share your thoughts what would be the best way to deal with Name column in that case.
About this issue
- Original URL
- State: closed
- Created 3 years ago
- Reactions: 1
- Comments: 17 (17 by maintainers)
If it works, let’s go with it. What you’ve described is exactly, what we want to achieve. We still need to cover new cases sorting and filtering for mixed translation, but that should be manageable. Can you open a PR with your proposal, so we can see the build?
Nonetheless, I’m wondering what is the change between your proposal and mine (#13653) that your works and mine not (despite fact, that I hardcoded
en_US)So, folks, I’ve tested proposals given and sample solutions you may find in related PoCs.
I’ve run performance tests, and here are my numbers:
Here are numbers:
So the conclusion is that both solutions have better performance, but lazy translations win. The problem is that both solutions would introduce the regression because it won’t correctly sort products in not default locale. The following scenarios already cover this issue:
https://github.com/Sylius/Sylius/blob/56bf4199f4c63e0c9101b1e3eb42bd5e4553583a/features/product/managing_products/sorting_products.feature#L50-L65
In addition, we should write new scenarios, how it should behave with missing translations for both sorting and filtering. My stand is that we cannot remove the “name” field nor skip sorting and filtering features. I don’t see any possibility of how to provide mentioned features for lazy translations. However, I’m almost sure that it is possible with some custom translation join and/or grouping. Perhaps someone will prove me wrong.
So, for anyone willing to pick it from here, what is required to solve the issue:
I don’t feel like it’s even a question worth asking. I’d say that bug is a duplicate of this one, and it’s pretty obvious that admin’s preferred locale should not act as an automatic filter of what that admin sees.
Hey Jakub!
It would be superb to have this fix finally in codebase! I’m wondering how much impact will this have for the code and performance. We can skip later until we will provide some tooling for that. The first part is more important. I can see, that we’ve been using leftjoin until 2017. You can find this change
in this PR: https://github.com/Sylius/Sylius/pull/7647/
The problem is, that we didn’t provide any tests back then. Perhaps reverting this change may be a good step forward. However, I would start with a reproduction scenario, so we can be sure that we are fixing the correct stuff. Then, let’s try your proposal and see if the build is green.
According to the
Namecolumn issue. Sylius was meant to print default translation in such a case and this is behaviour I would expect. The strange thing is that it should work like this right now. This is something, that we should cover with a scenario as well.