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

Migrate localization page #9116

Merged
merged 28 commits into from Jun 19, 2018

Conversation

@sarjon
Member

sarjon commented May 23, 2018

Questions Answers
Branch? develop
Description? Migrates "Improve > International > Localization" page to Symfony
Type? new feature
Category? CO
BC breaks? yes (overrides)
Deprecations? no
Fixed ticket? n/a
How to test? Access "Improve > International > Localization" and it should work the same as legacy page.

This change is Reviewable

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented May 23, 2018

@sarjon this need a rebase and probably some refactoring, I'll review tomorrow :)

</div>
<div class="form-group">
{{ ps.label_with_help(('Default currency'|trans), ('The default currency used in your shop.'|trans({}, 'Admin.Shopparameters.Help'))) }}

This comment has been minimized.

@LouiseBonnard

LouiseBonnard May 24, 2018

Contributor

The default currency used in your shop. must be attached to the Admin.International.Help domains.

This comment has been minimized.

@sarjon

sarjon May 24, 2018

Member

yup, thanks. :)

This comment has been minimized.

@LouiseBonnard

LouiseBonnard May 24, 2018

Contributor

You're welcome, Šarūnas!

@@ -0,0 +1,178 @@
<?php
/**

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 25, 2018

Contributor

wrong header

This comment has been minimized.

@sarjon

sarjon May 25, 2018

Member

oh gosh... 🤦‍♂️

$apiUrl = $this->configuration->get('_PS_API_URL_');
$localizationPackUrl = $apiUrl.'/localization/'.$psVersion.'/'.$countryIsoCode.'.xml';
$pack = $this->tools->getFileContents($localizationPackUrl);

This comment has been minimized.

@sarjon

sarjon May 25, 2018

Member

not sure if it's the best idea to add another method to Tools adapter class, what do you think @mickaelandrieu ?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 25, 2018

Contributor

why do we need something better than the native PHP function at first place? :/

This comment has been minimized.

@sarjon

sarjon May 26, 2018

Member

behind the scene (inside legacy Tools::file_get_contents) it actually uses curl_init function and some additional configuration to get localization data, so I don't want to duplicate it.
On the other hand, I tested it with native file_get_contents and it works fine. 🤔

*/
public function getFileContents($url)
{
return \Tools::file_get_contents($url);

This comment has been minimized.

@PierreRambaud

PierreRambaud May 25, 2018

Contributor

Use namespace please, LegacyTools is available :)

*
* @return bool|string
*/
public function getFileContents($url)

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 29, 2018

Contributor

we can't remove it, it's part of public API. But you can deprecate it.

This comment has been minimized.

@sarjon

sarjon May 30, 2018

Member

it's not part of public API, I have added it and now removed as it's not needed anymore.

@@ -24,17 +24,18 @@
* International Registered Trademark & Property of PrestaShop SA
*/
namespace PrestaShop\PrestaShop\Core\Localization\Pack;
namespace PrestaShop\PrestaShop\Core\Localization\Pack\Factory;
use LocalizationPack;

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 29, 2018

Contributor

you cant' use object model directly in Core classes. Rely on an Adapter instead.

This comment has been minimized.

@sarjon

sarjon May 30, 2018

Member

yup. :) I found that there's adapter aliases for legacy object models under PrestaShop\PrestaShop\Adapter\Entity namespace, is it okay if i use it?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 30, 2018

Contributor

Yes, thanks

@sarjon sarjon changed the title from [WIP] Migrate localization page to Migrate localization page May 30, 2018

@sarjon

This comment has been minimized.

Member

sarjon commented May 30, 2018

@mickaelandrieu this needs to be reviewed. :)

use Language;
class LanguageManager

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 30, 2018

Contributor

How about LanguageActivator instead? Let's remove every "manager", these classes have multiples responsabilities and we don't want it anymore.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 30, 2018

Contributor

Also would you mind to create LanguageActivatorInterface in Core/Language/ so when we will remove the adapter we can rely on the interface :)

This comment has been minimized.

@sarjon

sarjon May 30, 2018

Member

agree, naming things is hard. :/

{
$errors = [];
if ($this->validateConfiguration($config)) {

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 30, 2018

Contributor

it's not related to this pull request, but shouldn't we add a generic error when the configuration is invalid?

This comment has been minimized.

@sarjon

sarjon May 30, 2018

Member

sure, i could add it, but what this generic error should say?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 30, 2018

Contributor

I don't know, I'll ask @louise asap: don't change anything, we'll improve this behavior later :)

@mickaelandrieu

Good job, some refactoring needed :)

use Language;
class LanguageManager

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 30, 2018

Contributor

Also would you mind to create LanguageActivatorInterface in Core/Language/ so when we will remove the adapter we can rely on the interface :)

* Class LocalizationPackImportConfig is value object which is responsible
* for storing localization pack configuration for import
*/
final class LocalizationPackImportConfig

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 30, 2018

Contributor

this needs an interface (using PHPStorm you can generate it => https://www.jetbrains.com/help/phpstorm/extract-interface.html)

This comment has been minimized.

@sarjon

sarjon May 30, 2018

Member

it's a value object, do we need interface for it anyway?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 30, 2018

Contributor

yes because making this class final forbid the modification, so we must alllow developers to provide their own.

return [];
}
/**

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 30, 2018

Contributor

Thinking about it, we could use a trait (ok, I don't like traits but...).

Don't change it for now, I need time to decide if we introduce a new trait :)

This comment has been minimized.

@PierreRambaud

PierreRambaud May 30, 2018

Contributor

Or an abstract class it is for TranslatorAwareType (I don't like traits too ><)

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 30, 2018

Contributor

this is probably the worst use case to use mother/child class in OOP: extends a class when you have nothing related between child and his parent.

So, no no no ^^

This comment has been minimized.

@PierreRambaud

PierreRambaud May 31, 2018

Contributor

They are sharing translation love ❤️ (:trollface:)

{
/**
* Show localization settings page
*

This comment has been minimized.

@sarjon

sarjon May 30, 2018

Member

it seems that annotations are not merged into develop yet.

This comment has been minimized.

@PierreRambaud

PierreRambaud May 30, 2018

Contributor

yes, we will maybe merge 1.7.4.x into develop tomorrow :) (I need to)

*
* @param Request $request
*
* @return RedirectResponse

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 30, 2018

Contributor

use annotations instead :)

*
* @return array
*/
private function getLocalizationPackChoices()

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 30, 2018

Contributor

Extract this private function to a final class (and interface) and inject it instead: you will simplify A LOT your form type).

Something like LocalisationPackChoices with all() public function

'choices' => $this->getLanguageChoices(),
'attr' => [
'data-toggle' => 'select2',
'data-minimumResultsForSearch' => '7',

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 30, 2018

Contributor
- data-minimumResultsForSearch
+ data-minimum-results-for-search

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 30, 2018

Contributor

Same update everywhere please 👍

;
}
/**

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 30, 2018

Contributor

You don't need to inject the world in this formtype :)

What I suggest here

  • move every private function to his own "Provider/Repository": each one of them MUST return an array
  • so the constructor of this formType become __construct(array $languageChoices, array $countryChoices, array $currencyChoices, array $currencyChoices, array $timezoneChoices) { /* ... */}

In fact, it's ok to have private function in form types "if" you don't inject new dependencies only to be used in private functions: your private functions are about internal changes of your class: if you change something not related in it, this probably means this function should not be here :)

This comment has been minimized.

@sarjon

sarjon May 31, 2018

Member

hmm, should we have some generic interface under PrestaShopBundle\Form\Admin namespace like ChoiceValueProviderInterface with getChoices(): array method so it could be reused multiple times (in this PR i need 5 different choice providers). what do you think @mickaelandrieu ?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 6, 2018

Contributor

no, because I don't see why values of the form should be part of the contract.... and in this case, which "choices" should be in "getChoices"? $languageChoices, $countryChoices, $currencyChoices or $timezoneChoices?

We usually rely on interface to protect an implementation from his dependencies, but here we inject the dependencies so we are already protected: we can already type hint on array. Am I missing something here?

This comment has been minimized.

@sarjon

sarjon Jun 6, 2018

Member

Ok, maybe you are right, but here's and example in code of what I meant. :)

Example choices provider for language.

final class LanguageChoicesProvider implements ChoiceValueProviderInterface {
    public function __construct(/** some dependencies needed to get language choices */) {
        //...
    }

    public function getChoices(): array {
        // logic to build choices array
    }
}

How it would look in yaml form_type definition. We only pass arrays to form type instance.

    form.type.localization_configuration:
        class: 'PrestaShopBundle\Form\Admin\Improve\International\Localization\LocalizationConfigurationType'
        arguments:
            - '@prestashop.adapter.legacy.context'
            - '@=service("prestashop.bundle.form.language_choice_provider").getChoices()'
            - '@=service("prestashop.bundle.form.country_choice_provider").getChoices()'
            - '@=service("prestashop.bundle.form.currency_choice_provider").getChoices()'
            - '@=service("prestashop.bundle.form.timezone_choice_provider").getChoices()'
        tags:
            - { name: form.type }

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 6, 2018

Contributor

Oh! interesting :) This can formalize a constraint on getChoices ... can you add an issue on prestafony-project with this proposal? I have the feeling we should discuss it, but not in this pull request.

Still, I'm not sure yet of the gain to introduce the interface: does it make the code more testable, easier to understand or to extend? Are these arrays need to be strictly called "choices" or can they be used in other contexts?

@@ -0,0 +1,25 @@
services:

This comment has been minimized.

@mickaelandrieu
@@ -3,3 +3,4 @@ imports:
- { resource: core/configuration.yml }

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 30, 2018

Contributor

I don't know if you were of it, but you could do

  • { resource: core/*.yml }

instead.

Don't change it here, we'll do another pull request to improve all imports later.

if (!$xml = @simplexml_load_string($file)) {
if ($pack instanceof SimpleXMLElement) {
$xml = $pack;
} elseif (!$xml = @simplexml_load_string($pack)) {

This comment has been minimized.

@PierreRambaud

PierreRambaud May 30, 2018

Contributor

There is a libxml_clear_errors, instead using @ you can use libxml_use_internal_errors(true) : http://php.net/manual/fr/function.libxml-use-internal-errors.php

This comment has been minimized.

@sarjon

sarjon May 30, 2018

Member

yeah, but maybe this improvement should be part of another PR?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 30, 2018

Contributor

Agree this will change the behavior and I don't want the migration blocked by an issue not "strictly" related to it.

The good thing is now we have class with only one purpose, it will be easy to improve each part, including your interesting suggestion @PierreRambaud

*/
public function import(LocalizationPackImportConfig $config)
{
$errors = $errors = $this->checkConfig($config);

This comment has been minimized.

@PierreRambaud

PierreRambaud May 30, 2018

Contributor

Duplicate $errors

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 30, 2018

Contributor

wow! how i could miss it o_O

return null;
}
if (false === $xml) {

This comment has been minimized.

@PierreRambaud

PierreRambaud May 30, 2018

Contributor

If you do nothing in if maybe use ternary

return false === $xml ? null : $xml;
public function getLocalizationPack($countryIso)
{
$psVersion = str_replace('.', '', $this->configuration->get('_PS_VERSION_'));
$psVersion = substr($psVersion, 0, 2);

This comment has been minimized.

@PierreRambaud

PierreRambaud May 30, 2018

Contributor

Hum, what is the result if we are in version 1.12.1.0?

This comment has been minimized.

@sarjon

sarjon May 30, 2018

Member

well, it definitely wouldnt work.
however, localization API now supports 2 digits only (16 for 1.6.x, 17 for 1.7.x) and 1.10+ Prestashop is not coming any time soon (or maybe even never 😃 ) so i think we should keep same logic as it was in legacy code. what do you think?

This comment has been minimized.

@PierreRambaud

PierreRambaud May 31, 2018

Contributor

@eternoendless @mickaelandrieu Wdty? Could be a surprise bug in the future :/

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 6, 2018

Contributor

I have the feeling that someone has already done that... isn't @michaelKaefer? :)

=> https://github.com/PrestaShop/PrestaShop/pull/9154/files#diff-4f0928deabb59e9c6fd5e5c675628d61R83

Does it fix the bug you are discussing?

This comment has been minimized.

@sarjon

sarjon Jun 6, 2018

Member

not really, in this case http://api.prestashop.com/localization/{psVersion}/{countryIso}.xml is used to get localization pack data. {psVersion} is passed as 17 and not as 1.7 (same for 1.6, it goes as 16 value).
In legacy part it used to take first 2 string letters from version, so i kept it that way, should we change it?

p.s. maybe this major version method could be extracted into separate service as it could be reused elsewhere?

This comment has been minimized.

@michaelKaefer

michaelKaefer Jun 6, 2018

Contributor

@mickaelandrieu @sarjon So the existing method could be used (just replace the "." with an empty string) but it is definitly in the wrong place (a controller). What would be the correct place? Should I create a service for retrieving the actual PS version (in various formats like "1.7.4.1" or "1.7" or "17") like sarjon suggested? I am definitly motivated to do it if you tell me the correct place where to put the code 👍

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 6, 2018

Contributor

sure! In Core/Foundation/Version class (you may need to create it).

I'll complete an issue in prestafony project with some hints to help you, there is no hurry here ;)

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 6, 2018

Contributor

@sarjon, this task is not required in this contribution I consider your work done after a final rebase (I've just merged a migration, so the Link class needs to be updated).

This comment has been minimized.

@michaelKaefer

michaelKaefer Jun 6, 2018

Contributor

@mickaelandrieu Ok, thanks! I'll write you 👍

if ($localizationPacks) {
foreach ($localizationPacks as $pack) {
$iso = (string) $pack->iso;

This comment has been minimized.

@PierreRambaud

PierreRambaud May 30, 2018

Contributor

Variables are useless $choices[(string) $pack->iso] = (string) $pack->name; and not sure casting to string is useful

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented May 31, 2018

it's the same method as above, maybe refactoring by create a method setActive($status);, is it normal to settings this value to 1 or 0 and not to true or false?

Sadly, I think it's a "tiny int" stored in database: I don't like it neither :/

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Jun 6, 2018

it's the same method as above, maybe refactoring by create a method setActive($status);, is it normal to settings this value to 1 or 0 and not to true or false?

Sadly, I think it's a "tiny int" stored in database: I don't like it neither :/

In fact, MySQL boolean field is a tinyint(1) (YEAH FUCK LOGIC), so we can use true and false that should be automatically converted to tinyint :)

CREATE TABLE `test_prestashop`.`test_boolean` ( `test` BOOLEAN NOT NULL ) ENGINE = InnoDB;

INSERT INTO `test_boolean` (`test`) VALUES ('1');
INSERT INTO `test_boolean` (`test`) VALUES (1);
INSERT INTO `test_boolean` (`test`) VALUES (TRUE);

The dump:

CREATE TABLE `test_boolean` (
  `test` tinyint(1) NOT NULL
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;

This is one reason why I hate MySQL :trollface:

$localizationPack->loadLocalisationPack(
$pack,
$config->getContentToImport(),
$installMode = false,

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 6, 2018

Contributor

not need to declare this variable if you dont use it :)

private function getCurrencyChoices()
{
$currencies = $this->currencyDataProvider->getCurrencies(
$asObjects = false,

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 6, 2018

Contributor

same here, you don't need the variables... I guess your intent is to describe the function :)

Once these function will be moved to the providers, you won't need it anymore :)

@sarjon

This comment has been minimized.

Member

sarjon commented Jun 6, 2018

@mickaelandrieu i've addressed most of the comments from the review except some that are still in discussion. :)

@sarjon

This comment has been minimized.

Member

sarjon commented Jun 19, 2018

@mickaelandrieu does this need QA approval again? its rebased and uses Version service.

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 19, 2018

Thanks @sarjon and thanks everyone for your reviews 👍

@mickaelandrieu mickaelandrieu merged commit edae842 into PrestaShop:develop Jun 19, 2018

1 of 3 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
code-review/reviewable 62 files, 28 discussions left (mickaelandrieu, PierreRambaud, sarjon)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mickaelandrieu mickaelandrieu referenced this pull request Jun 26, 2018

Closed

Roadmap migration (1.7.5) #10

26 of 40 tasks complete

@eternoendless eternoendless added this to the 1.7.5.0 milestone Aug 10, 2018

@mickaelandrieu mickaelandrieu referenced this pull request Sep 5, 2018

Closed

Symfony migration roadmap for 1.7.5 #10301

27 of 40 tasks complete

@matks matks added the migration label Sep 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment