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

Remove usage of legacy locale parameter #374

Open
wants to merge 9 commits into
base: 1.10
Choose a base branch
from
Open

Remove usage of legacy locale parameter #374

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
eb294d2
Remove usage of legacy locale parameter
loic425 Mar 2, 2022
9d5e422
Improve DX with existing parameters
loic425 Mar 3, 2022
c5ab2ed
Fix README
loic425 Mar 3, 2022
9a9cbbe
Use kernel parameters on configuration reference
loic425 Mar 3, 2022
9acdc42
Fix PHPStan errors
loic425 Mar 3, 2022
b9dec16
Add missing tests with default values
loic425 Mar 3, 2022
db1f026
Update UPGRADE.md
loic425 Mar 3, 2022
1c26d09
Add some deprecation notices
loic425 Mar 3, 2022
9737c93
Improve translation configuration for enabled locales
loic425 Mar 3, 2022
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
35 changes: 35 additions & 0 deletions UPGRADE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,38 @@
## UPGRADE FOR `1.9.x`

### FROM `1.8.x` TO `1.9.x`

- `translations`:

If you are using translations, the default locale provider `sylius.translation_locale_provider.immutable` is using two new translation options.

```yaml
sylius_resource:
translation:
enabled_locales: []
default_locale: null
```

Before `1.9.x` it was configured with `%locale%` parameter.
If you still use this `%locale%`parameter, you can configure translation like this:

```yaml
sylius_resource:
translation:
enabled_locales: ['%locale%']
default_locale: '%locale%'
```

If you use `%kernel.default_locale%` and `%kernel.enabled_locales%` parameters, you can configure translation like this:


```yaml
sylius_resource:
translation:
enabled_locales: '%kernel.enabled_locales%'
default_locale: '%kernel.default_locale%'
```
loic425 marked this conversation as resolved.
Show resolved Hide resolved

## UPGRADE FOR `1.7.x`

### FROM `1.6.x` TO `1.7.x`
Expand Down
17 changes: 17 additions & 0 deletions docs/reference.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,23 @@

```yaml
sylius_resource:
mapping:
paths: ['%kernel.project_dir%/src/Entity'] # Used for Routes with PHP attributes
translation:
enabled: true
enabled_locales: '%kernel.enabled_locales%'
default_locale: '%kernel.default_locale%'
locale_provider: 'sylius.translation_locale_provider.immutable'
settings:
paginate: ~
limit: ~
allowed_paginate: [10, 20, 30]
default_page_size: 10
sortable: false
sorting: ~
filterable: false
criteria: ~
state_machine_component: winzou # or symfony
Comment on lines +5 to +21
Copy link
Member

Choose a reason for hiding this comment

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

I understand all of these parameters were missing, but it would be more ordered if we add only enabled_locales and default_locale here and the rest of them in a separate PR 😄 #beingPettyIsMyPassion

resources:
app.book:
driver: doctrine/orm
Expand Down
5 changes: 5 additions & 0 deletions src/Bundle/DependencyInjection/Configuration.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,11 @@ private function addTranslationsSection(ArrayNodeDefinition $node): void
->arrayNode('translation')
->canBeDisabled()
->children()
->arrayNode('enabled_locales')
->defaultValue([])
->prototype('scalar')->end()
->end()
->scalarNode('default_locale')->defaultNull()->end()
->scalarNode('locale_provider')->defaultValue('sylius.translation_locale_provider.immutable')->cannotBeEmpty()->end()
->end()
->end()
Expand Down
65 changes: 65 additions & 0 deletions src/Bundle/DependencyInjection/SyliusResourceExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ public function load(array $configs, ContainerBuilder $container): void
$loader->load('services/integrations/translation.xml');

$container->setAlias('sylius.translation_locale_provider', $config['translation']['locale_provider'])->setPublic(true);
$this->createTranslationParameters($config, $container);
}

$container->setParameter('sylius.resource.mapping', $config['mapping']);
Expand Down Expand Up @@ -135,4 +136,68 @@ private function loadResources(array $loadedResources, ContainerBuilder $contain
}
}
}

private function createTranslationParameters(array $config, ContainerBuilder $container): void
{
$this->createEnabledLocalesParameter($config, $container);
$this->createDefaultLocaleParameter($config, $container);
}

private function createEnabledLocalesParameter(array $config, ContainerBuilder $container): void
{
$enabledLocales = $config['translation']['enabled_locales'];

if (count($enabledLocales) > 0) {
$container->setParameter('sylius.resource.translation.enabled_locales', $enabledLocales);

return;
}

if ($container->hasParameter('locale')) {
$container->setParameter('sylius.resource.translation.enabled_locales', [$container->getParameter('locale')]);

Zales0123 marked this conversation as resolved.
Show resolved Hide resolved
return;
}

if ($container->hasParameter('kernel.enabled_locales')) {
$kernelEnabledLocales = (array) $container->getParameter('kernel.enabled_locales');

if (count($kernelEnabledLocales) > 0) {
$container->setParameter('sylius.resource.translation.enabled_locales', $container->getParameter('kernel.enabled_locales'));

return;
}
}

$container->setParameter('sylius.resource.translation.enabled_locales', ['en']);
}

private function createDefaultLocaleParameter(array $config, ContainerBuilder $container): void
{
$defaultLocale = $config['translation']['default_locale'];

if (is_string($defaultLocale)) {
$container->setParameter('sylius.resource.translation.default_locale', $defaultLocale);

return;
}

if ($container->hasParameter('locale')) {
$container->setParameter('sylius.resource.translation.default_locale', $container->getParameter('locale'));

Zales0123 marked this conversation as resolved.
Show resolved Hide resolved
return;
}

if ($container->hasParameter('kernel.default_locale')) {
$kernelDefaultLocale = $container->getParameter('kernel.default_locale');

if (is_string($kernelDefaultLocale)) {
$container->setParameter('sylius.resource.translation.default_locale', $kernelDefaultLocale);

return;
}
}

$container->setParameter('sylius.resource.translation.default_locale', 'en');
Copy link
Member

Choose a reason for hiding this comment

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

As far as I see, previously it was only a %locale% parameter. Dit it fallback previously to 'en' too? If it didn't, maybe we should throw some exception to not assume anything that was not assumed before? 🤔 I just would like to keep it as close to the previous implementation as possible 🖖

Copy link
Contributor

@vvasiloi vvasiloi Mar 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is a BC layer, so throwing an exception here defies the purpose of this code.
It could trigger a deprecation error though, warning that this default will be removed and that it should be configured explicitly.

Copy link
Contributor

Choose a reason for hiding this comment

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

The en is coming from the default value of locale parameter configured in Sylius, Sylius Standard and Sylius Plugin Skeleton, which all should be removed after this PR is merged.

Copy link
Member

Choose a reason for hiding this comment

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

I have mixed feelings about it as well. What is more, it is bumped by the fact that we do not have tests for more than one config present here and a description of winning strategy (for now it is: 1. new resource config, 2. previous parameter, 3. Symfony parameter, 4. "en").

Another concern is that we are replacing the old parameter with the new one entirely, but both configurations may (and probably should) work together. This parameter is used in several places. This is the screenshot from the current version of Sylius-Standard:
image

So now - the best config would be to have $config['translation']['default_locale'] defined together with the %locale% parameter defined anyway - the application will work, and no deprecation warning will be triggered (as we have early return). Also, the application will work with the %locale% parameter defined without our new config, but the deprecation will be triggered. If nothing is defined or only our new config - the whole application will collapse. Am I missing something?

What I would recommend is as follows - the %locale% parameter should be declared together with the %sylius.resource.translation.default_locale% if not present based on the new config (that could have "en" defined as default in config tree). If the%locale% parameter is defined together with our new config, then the error/deprecation warning should be triggered(I'm not sure about which one). The last question remains - should the "%locale%" be overridden by our config, or should it override our default locale? I would say the latter, but I'm open to discussing it.

Anyway, I'm also lacking the cases I've described exposed as configuration tests + test drive with the current Sylius and/or Sylius-Standard to check the correctness of the new solution.

Copy link
Contributor

@vvasiloi vvasiloi Mar 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I have revisited this and I agree that it needs some changes.
The goal is to get rid of the locale parameter and use the new configuration.
This means that internally, all Sylius packages, should remove the locale parameter and use the new configuration.
The only way a locale parameter will still exist, is if it's still defined in the end user projects.
That's where the BC layer comes in action and uses the value of the locale parameter to feed the new configuration, but only if it wasn't already explicitly set by the user.
In this case, a deprecation error should be triggered encouraging the user to migrate from using the locale parameter to the new configuration.
Here are the possible scenarios and the outcomes I think they should have:

config defined config undefined
locale defined 1. Use the config and suggest the user to remove the redundant parameter. 2. Use the parameter value as default config values and trigger a deprecation error to encourage the user to explicitly set the config values and remove the parameter.
locale undefined 3. Use the config. This is the desired outcome. 4. Use the default configuration, which is set to kernel.default_locale and kernel.enabled_locales. This will be the default behavior in new installs.

There's a 5th scenario, which is a combination of the 4th scenario with Symfony 4.4, which does not have the kernel.enabled_locales parameter.
There is another important detail to consider in this case: when kernel.enabled_locales is empty, all locales should be enabled.
In the Resource Bundle context this is not a viable solution, so we should stick to the current behavior and only use the default locale as the only available locale by default.
This means that, when kernel.enabled_locales is empty (normal behavior) or is not defined (BC layer), the default value of sylius.resource.translation.enabled_locales should be and array with only sylius.resource.translation.default_locale.

To achieve the above, the following modifications must be made to the currently proposed changes:

  1. Add note about the what happens when kernel.enabled_locales is empty;
  2. Define the default locale parameter first, as it should always be
  3. sylius.resource.translation.enabled_locales should fallback to sylius.resource.translation.default_locale (instead of the current ['en'] if no other options were found;
  4. $container->setParameter('sylius.resource.translation.default_locale', 'en'); should be removed and an error should be thrown if the default locale was not set any other way

WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

@vvasiloi Thank you, this is an awesome explanation. I think your suggested change is perfect.

}
}
4 changes: 2 additions & 2 deletions src/Bundle/Resources/config/services/integrations/translation.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@

<service id="sylius.translation_locale_provider.immutable" class="Sylius\Component\Resource\Translation\Provider\ImmutableTranslationLocaleProvider">
<argument type="collection">
<argument>%locale%</argument>
<argument>%sylius.resource.translation.enabled_locales%</argument>
vvasiloi marked this conversation as resolved.
Show resolved Hide resolved
</argument>
<argument>%locale%</argument>
<argument>%sylius.resource.translation.default_locale%</argument>
</service>

<service id="sylius.translation.translatable_listener.doctrine.orm" class="Sylius\Bundle\ResourceBundle\EventListener\ORMTranslatableListener">
Expand Down
62 changes: 55 additions & 7 deletions src/Bundle/Tests/Configuration/ConfigurationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use Matthias\SymfonyConfigTest\PhpUnit\ConfigurationTestCaseTrait;
use PHPUnit\Framework\TestCase;
use Sylius\Bundle\ResourceBundle\DependencyInjection\Configuration;
use Symfony\Component\Config\Definition\ConfigurationInterface;

class ConfigurationTest extends TestCase
{
Expand All @@ -24,7 +25,7 @@ class ConfigurationTest extends TestCase
/**
* @test
*/
public function it_does_not_break_if_not_customized()
public function it_does_not_break_if_not_customized(): void
{
$this->assertConfigurationIsValid(
[
Expand All @@ -36,7 +37,7 @@ public function it_does_not_break_if_not_customized()
/**
* @test
*/
public function it_has_default_mapping_paths()
public function it_has_default_mapping_paths(): void
{
$this->assertProcessedConfigurationEquals(
[
Expand All @@ -56,7 +57,7 @@ public function it_has_default_mapping_paths()
/**
* @test
*/
public function its_mapping_paths_can_be_customized()
public function its_mapping_paths_can_be_customized(): void
{
$this->assertProcessedConfigurationEquals(
[
Expand All @@ -78,7 +79,54 @@ public function its_mapping_paths_can_be_customized()
/**
* @test
*/
public function it_has_default_authorization_checker()
public function it_has_default_translation(): void
{
$this->assertProcessedConfigurationEquals(
[
[],
],
[
'translation' => [
'enabled' => true,
'enabled_locales' => [],
'default_locale' => null,
'locale_provider' => 'sylius.translation_locale_provider.immutable',
],
],
'translation'
);
}

/**
* @test
*/
public function its_translation_can_be_customized(): void
{
$this->assertProcessedConfigurationEquals(
[
['translation' => [
'enabled' => false,
'enabled_locales' => ['fr', 'pl'],
'default_locale' => 'fr',
'locale_provider' => 'app.locale_provider.custom',
]],
],
[
'translation' => [
'enabled' => false,
'enabled_locales' => ['fr', 'pl'],
'default_locale' => 'fr',
'locale_provider' => 'app.locale_provider.custom',
],
],
'translation'
);
}

/**
* @test
*/
public function it_has_default_authorization_checker(): void
{
$this->assertProcessedConfigurationEquals(
[
Expand All @@ -92,7 +140,7 @@ public function it_has_default_authorization_checker()
/**
* @test
*/
public function its_authorization_checker_can_be_customized()
public function its_authorization_checker_can_be_customized(): void
{
$this->assertProcessedConfigurationEquals(
[
Expand All @@ -106,7 +154,7 @@ public function its_authorization_checker_can_be_customized()
/**
* @test
*/
public function its_authorization_checker_cannot_be_empty()
public function its_authorization_checker_cannot_be_empty(): void
{
$this->assertPartialConfigurationIsInvalid(
[
Expand All @@ -116,7 +164,7 @@ public function its_authorization_checker_cannot_be_empty()
);
}

protected function getConfiguration()
protected function getConfiguration(): ConfigurationInterface
{
return new Configuration();
}
Expand Down
Loading