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 languages list #10839

Merged
merged 20 commits into from Nov 22, 2018

Conversation

@sarjon
Member

sarjon commented Oct 3, 2018

Questions Answers
Branch? develop
Description? Migration of "Improve > International > Localization > Languages" list.
Type? new feature
Category? CO
BC breaks? yes
Deprecations? no
Fixed ticket? #10579
How to test? Filtering, sorting and pagination should work the same. Note: it does not migrate actions like Delete, Change status and etc. /admin-dev/index.php/improve/international/languages/

This change is Reviewable

@sarjon

This comment has been minimized.

Member

sarjon commented Oct 5, 2018

waiting for #10864 to be merged as images are used in this list.

@matks matks referenced this pull request Oct 8, 2018

Open

Migrate "International > Localization > Languages" page #10579

1 of 3 tasks complete

@sarjon sarjon force-pushed the sarjon:migrate/languages-list branch from 48d9153 to f848075 Oct 10, 2018

@sarjon sarjon changed the title from [WIP] Migrate languages list to Migrate languages list Oct 10, 2018

@sarjon

This comment has been minimized.

Member

sarjon commented Oct 12, 2018

@matks ready for review. :)

$languageData = $this->doctrineLanguageDataFactory->getData($searchCriteria);
$modifiedRecords = $this->applyModification(
$languageData->getRecords()->all()

This comment has been minimized.

@matks

matks Oct 16, 2018

Contributor

We cannot use $languageData->getRecords() as an iterator ?

This comment has been minimized.

@sarjon

sarjon Oct 16, 2018

Member

you can to iterate over records, but it does not implement ArrayAccess, so you cannot do something like: $languages[$i]['flag'] = ...

This comment has been minimized.

@matks

matks Oct 16, 2018

Contributor

Argh 😅

private $contextShopId;
/**
* @param ImageTagSourceParserInterface $imageTagSourceParser

This comment has been minimized.

@matks

matks Oct 16, 2018

Contributor

Yeah ! 1st reuse of ImageTagSourceParser 🙂 !

class: 'PrestaShop\PrestaShop\Core\Grid\Definition\Factory\LanguageGridDefinitionFactory'
parent: 'prestashop.core.grid.definition.factory.abstract_grid_definition'
arguments:
- "@=service('router').generate('admin_common_reset_search', {'controller': 'language', 'action': 'index'})"

This comment has been minimized.

@matks

matks Oct 16, 2018

Contributor

😅 I dont know whether this is right
We have a service definition that relies on:

  • the router method (ok, this one is not going to change a lot)
  • a route configuration, from a YAML file

I'm afraid few people, if they modify a routing YAML file, will think "ok, I got to modify the service definition file too".
It consequently looks fragile to me.

However if we can make sure a test will find out if we break one service definition, I'm ok 😀

Here's one idea (however feel free to suggest others): we could create a test suite that basically parses the Symfony Container (probably through a Symfony Command) and tries to get every service. If the service definition is broken, this test would fail.

It would look like this:

$errors = [];
$container = $this->getContainer();
foreach($container->getAllServices as $serviceName) {
    try {
        $myService = $container->get($serviceName);
    } catch (ServiceCannotBeBuiltException $e) {
        $errors[] = $e;
    }
}

if (!empty($errors)) {
    // test has fail, print the list of services which cannot be built with the reasons
}

However this would only work for public services. Need to find an idea for private services too 😉

This comment has been minimized.

@sarjon

sarjon Oct 16, 2018

Member

actually i noticed same issue and also the problem that you have to inject these params every time you create grid definition. So basically, what i did is updated SearchAndResetType as it's the reason why we need to inject these params in the first place.

You can check at this line https://github.com/PrestaShop/PrestaShop/pull/10916/files#diff-74412ed3073d7a554856e4bdf755a4efR315 wdyt?

This comment has been minimized.

@matks

matks Oct 17, 2018

Contributor

I'm really not a fan of passing reset_route_params. I'm afraid we start adding more and more array-driven configuration options until we get something like this legacy controller config. Big config arrays that look terrifying for new prestashop developers as they think "I have to pass ALL these options ? what are them ? what are possible values ?" and if they input the wrong values, the BO fails.

I might be traumatized by previous experiences 😅 so let's ask around for opinions
@Quetzacoalt91 @PierreRambaud @jolelievre

This comment has been minimized.

@sarjon

sarjon Oct 17, 2018

Member

thats a valid point.

</div>
</div>
{% block language_listing %}

This comment has been minimized.

@matks

matks Oct 16, 2018

Contributor

👍

Show resolved Hide resolved tests/Unit/Core/Image/Parser/ImageTagSourceParserTest.php

@sarjon sarjon force-pushed the sarjon:migrate/languages-list branch from f848075 to 7aed331 Oct 23, 2018

@matks

This comment has been minimized.

Contributor

matks commented Oct 23, 2018

Only remaining question is: what do we do about https://github.com/PrestaShop/PrestaShop/pull/10839/files#r225529114 ?

@matks matks referenced this pull request Oct 23, 2018

Merged

Migrate customers listing #10916

@sarjon

This comment has been minimized.

Member

sarjon commented Oct 23, 2018

i thought about this and i think its okay to have array configuration for FormType, right? all form types are configured that way, wdyt? its regarding this implementation https://github.com/PrestaShop/PrestaShop/pull/10916/files#diff-74412ed3073d7a554856e4bdf755a4efR315

@matks

This comment has been minimized.

Contributor

matks commented Oct 23, 2018

@sarjon You mean reset_route_params ?

@sarjon

This comment has been minimized.

Member

sarjon commented Oct 24, 2018

You mean reset_route_params ?

yes, its part of form type and they are configured using arrays.

@sarjon sarjon force-pushed the sarjon:migrate/languages-list branch from 7aed331 to 52fb597 Nov 5, 2018

@matks

This comment has been minimized.

Contributor

matks commented Nov 8, 2018

@sarjon Back to this issue. So you suggest to update LanguageGridDefinitionFactory, inject only the router and have $resetUrl and $redirectUrl being built in the class, using params passed by configuration ?

@sarjon

This comment has been minimized.

Member

sarjon commented Nov 12, 2018

not really, we dont need to inject anytging into LanguageGridDefinitionFactory. All we have to do is configure FormType with route names. See below:

// Before
->add((new Filter('actions', SearchAndResetType::class))
    ->setTypeOptions([
        'attr' => [
            'data-url' => $this->resetUrl,   //  resetUrl is injected into constructor by genereting it in service definition like: "@=service('router').generate('admin_common_reset_search', {'controller': 'language', 'action': 'index'})"
            'data-redirect' => $this->redirectUrl, // same with redirectUrl
        ],
    ])
    ->setAssociatedColumn('actions')
)
// After
->add((new Filter('actions', SearchAndResetType::class))
    ->setTypeOptions([
        'reset_route' => 'admin_common_reset_search', // no dependencies injected, just SearchAndResetType configuration
        'redirect_route' => 'admin_languages_index', // same here
    ])
    ->setAssociatedColumn('actions')
)

@sarjon sarjon force-pushed the sarjon:migrate/languages-list branch from 52fb597 to 2564b05 Nov 12, 2018

@matks

This comment has been minimized.

Contributor

matks commented Nov 12, 2018

@sarjon I'm still not 100% convinced because I'm afraid people will get confused about what they need to pass in these arrays but this is the best solution we have so far
So ... approved 👍

@marionf marionf self-assigned this Nov 12, 2018

@marionf

This comment has been minimized.

Contributor

marionf commented Nov 13, 2018

It's ok for me

@TristanLDD is it ok for you ?

capture d ecran_608

@marionf marionf added the QA ✔️ label Nov 13, 2018

@sarjon sarjon force-pushed the sarjon:migrate/languages-list branch from fc2e928 to 26b31d6 Nov 21, 2018

@sarjon

This comment has been minimized.

Member

sarjon commented Nov 21, 2018

done. 👍

@sarjon

This comment has been minimized.

Member

sarjon commented Nov 22, 2018

@PierreRambaud once travis is green, can you merge? @tomas862 would like to use some services from this PR. :)

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Nov 22, 2018

@sarjon Of course, I will do it :)

@PierreRambaud PierreRambaud merged commit f370a9e into PrestaShop:develop Nov 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Nov 22, 2018

Thanks @sarjon nice work again!

@PierreRambaud PierreRambaud added this to the 1.7.6.0 milestone Nov 22, 2018

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