Skip to content
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

Jump to bottom

[ResourceBundle] Create Sylius translations type #4318

Merged
merged 1 commit into from
Mar 22, 2016
Merged

[ResourceBundle] Create Sylius translations type #4318

merged 1 commit into from
Mar 22, 2016

Conversation

tuka217
Copy link
Contributor

@tuka217 tuka217 commented Feb 26, 2016

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Fixed tickets #4146, #4147, #4256
License MIT
Doc PR -

@pjedrzejewski pjedrzejewski changed the title [WIP][ResourceBuble] Create Sylius translations type [WIP][ResourceBundle] Create Sylius translations type Feb 26, 2016
@pjedrzejewski pjedrzejewski added Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). New Feature and removed Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). labels Feb 26, 2016
@tuka217 tuka217 force-pushed the sylius-translations branch 8 times, most recently from 8c4e9f9 to 00fe2f9 Compare March 8, 2016 11:19
@tuka217 tuka217 force-pushed the sylius-translations branch 3 times, most recently from a7e689f to 8865c7f Compare March 9, 2016 11:27
pjedrzejewski
pjedrzejewski reviewed Mar 9, 2016
View reviewed changes
src/Sylius/Bundle/CoreBundle/Locale/TranslationLocaleProvider.php
@@ -37,7 +39,7 @@ public function __construct(LocaleContextInterface $localeContext)
*/
public function getCurrentLocale()
{
return $this->localeContext->getCurrentLocale();
return $this->container->get('sylius.context.locale')->getCurrentLocale();
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid a problem with circular reference. I now, that it isn't the best solution, but only this work.

Copy link
Contributor

Choose a reason for hiding this comment

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

After a discussion with @pjedrzejewski we agreed that the best solution for this issue would be to move getAvailableLocales method to another service AvailableTranslationLocalesProvider. It will make TranslationLocaleProvider not dependent on the database so the circular dependency issue would be solved. What do you think about that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, that should be better solution 👍

@tuka217 tuka217 force-pushed the sylius-translations branch 5 times, most recently from 4f4f0af to e174fa5 Compare March 10, 2016 12:25
@pjedrzejewski pjedrzejewski mentioned this pull request Mar 10, 2016
Closed
4 tasks
@tuka217 tuka217 force-pushed the sylius-translations branch 4 times, most recently from 0c5c401 to a6c1212 Compare March 14, 2016 15:20
pjedrzejewski
pjedrzejewski reviewed Mar 15, 2016
View reviewed changes
src/Sylius/Bundle/ResourceBundle/Provider/ArrayLocalesProvider.php
* @var array
*/
private $availableLocales;

Copy link
Member

Choose a reason for hiding this comment

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

Extra blank line.

lchrusciel
lchrusciel reviewed Mar 21, 2016
View reviewed changes
src/Sylius/Component/Resource/Provider/LocaleProvider.php
@@ -27,13 +27,20 @@ class LocaleProvider implements LocaleProviderInterface
private $fallbackLocale;

/**
* @var string|null
Copy link
Member

Choose a reason for hiding this comment

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

string only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? In constructor $defaultLocale can be null by default.

Copy link
Member

Choose a reason for hiding this comment

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

IIRC we used to define only type for properties. Just a thought.

@tuka217 tuka217 force-pushed the sylius-translations branch 2 times, most recently from af45644 to e95253e Compare March 21, 2016 10:31
@tuka217 tuka217 changed the title [WIP][ResourceBundle] Create Sylius translations type [ResourceBundle] Create Sylius translations type Mar 21, 2016
@tuka217 tuka217 force-pushed the sylius-translations branch 3 times, most recently from 701429e to 40d7733 Compare March 21, 2016 11:21
@NoResponseMate @tuka217
[ResourceBundle] Add ResourceTranslationType
19ca019 
[ResourceBundle] Create ResourceTranslationsSubscriber
[WebBundle] Add block for rendering sylius_translations_widget in forms.html.twig
[sylius] Change occurences of 'a2lix_translationsForms' to 'sylius_translations'

[CoreBundle] Add cofinguration for news locales providers, create AvailableTranslationLocalesProvider
[ResourceBundle] Add configuration for translation locales providers
[ResourceBundle] Create configuration tests for translation

[ResourceBundle] Change api tests, add new configuration test
koemeet
koemeet reviewed Mar 22, 2016
View reviewed changes
src/Sylius/Bundle/ResourceBundle/test/src/AppBundle/Entity/Book.php
{
use TranslatableTrait {
__construct as private initializeTranslationsCollection;
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't you forgetting to call initializeTranslationsCollection in the constructor?

Zales0123
Zales0123 reviewed Mar 22, 2016
View reviewed changes
src/Sylius/Bundle/CoreBundle/Provider/AvailableLocalesProvider.php
}

/**
* @return array
Copy link
Member

Choose a reason for hiding this comment

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

{@inheritdoc}

pjedrzejewski pushed a commit that referenced this pull request Mar 22, 2016
Merge pull request #4318 from tuka217/sylius-translations
2b0cbe6 
[ResourceBundle] Create Sylius translations type
@pjedrzejewski pjedrzejewski merged commit 2b0cbe6 into Sylius:master Mar 22, 2016
@pjedrzejewski
Copy link
Member

Really great work Ania! In separate PR, please apply small comments from @Zales0123 and also uninstall a2lix from composer.json's & Kernel to make sure it is not used anywhere. 👍

@Zales0123 Zales0123 mentioned this pull request Apr 14, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@tuka217 @pjedrzejewski @koemeet @pamil @Zales0123 @lchrusciel @Arminek @michalmarcinkowski @NoResponseMate