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

Allow overriding of every part of Grid templates #9281

Merged
merged 11 commits into from Sep 20, 2018

Conversation

Projects
None yet
5 participants
@sarjon
Member

sarjon commented Jul 8, 2018

Questions Answers
Branch? develop
Description? Adds column_content() twig macro for easier column rendering. This macro can be reused in different grid templates.
Type? new feature
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? n/a
How to test? Logs grid should be rendered the same.

This change is Reviewable

@mickaelandrieu mickaelandrieu changed the title from Add column_content() twig function to Add column_content() twig macro Jul 16, 2018

@sarjon sarjon changed the title from Add column_content() twig macro to Update grid templating Aug 16, 2018

@sarjon sarjon referenced this pull request Aug 25, 2018

Open

Checklist with Grid component improvements #68

0 of 12 tasks complete
@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Aug 30, 2018

Member

@mickaelandrieu it's rebased. 👍

however, im not really happy with performance. :/ when including template directly, like this:

{{ include('@PrestaShop/Admin/Common/Grid/Columns/'~column.type~'.html.twig', {'grid': grid, 'column': column, 'record': record}) }}

it's ~2x faster than new implementation with function:

{{ column_content(record, column, grid) }}

any ideas for improvement? 🤔

what i suggest doing is:

(new SomeColumn('column_id'))
    ->setName('My column name')
    ->setOptions([
        // ...
    ])
    // we may have some default rendering if custom template is not set
    // additionaly it would allow module developers to easily customize template as it's pretty straightforward
    ->setTemplate('/path/to/template/column.html.twig')
;

And we can get rid of type property for column as it is only used for figuring out template, so it's basically template name, but in different format, wdyt?

Member

sarjon commented Aug 30, 2018

@mickaelandrieu it's rebased. 👍

however, im not really happy with performance. :/ when including template directly, like this:

{{ include('@PrestaShop/Admin/Common/Grid/Columns/'~column.type~'.html.twig', {'grid': grid, 'column': column, 'record': record}) }}

it's ~2x faster than new implementation with function:

{{ column_content(record, column, grid) }}

any ideas for improvement? 🤔

what i suggest doing is:

(new SomeColumn('column_id'))
    ->setName('My column name')
    ->setOptions([
        // ...
    ])
    // we may have some default rendering if custom template is not set
    // additionaly it would allow module developers to easily customize template as it's pretty straightforward
    ->setTemplate('/path/to/template/column.html.twig')
;

And we can get rid of type property for column as it is only used for figuring out template, so it's basically template name, but in different format, wdyt?

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 31, 2018

Contributor
(new SomeColumn('column_id'))
    ->setName('My column name')
    ->setOptions([
        // ...
    ])
    // we may have some default rendering if custom template is not set
    // additionaly it would allow module developers to easily customize template as it's pretty straightforward
    ->setTemplate('/path/to/template/column.html.twig')
;

I repeat: A column is not aware of his representation and will never be. Stop trying to break the architecture :/

Let's remove the function and stay with the include then 👍

Also:

// additionaly it would allow module developers to easily customize template as it's pretty straightforward

like overrides, like hooks with display, like globals, like public properties, like weak typing, like weak function signature, like everything that make people blame us because they can't find a good developer for a customization or because they can't upgrade their shop.

Nothing is perfect, there's only trade offs: I don't remember whose telling me that ;)

And we can get rid of type property for column as it is only used for figuring out template, so it's basically template name, but in different format, wdyt?

We use it to select a template, but there is no hard bound between type property and a twig template.

Contributor

mickaelandrieu commented Aug 31, 2018

(new SomeColumn('column_id'))
    ->setName('My column name')
    ->setOptions([
        // ...
    ])
    // we may have some default rendering if custom template is not set
    // additionaly it would allow module developers to easily customize template as it's pretty straightforward
    ->setTemplate('/path/to/template/column.html.twig')
;

I repeat: A column is not aware of his representation and will never be. Stop trying to break the architecture :/

Let's remove the function and stay with the include then 👍

Also:

// additionaly it would allow module developers to easily customize template as it's pretty straightforward

like overrides, like hooks with display, like globals, like public properties, like weak typing, like weak function signature, like everything that make people blame us because they can't find a good developer for a customization or because they can't upgrade their shop.

Nothing is perfect, there's only trade offs: I don't remember whose telling me that ;)

And we can get rid of type property for column as it is only used for figuring out template, so it's basically template name, but in different format, wdyt?

We use it to select a template, but there is no hard bound between type property and a twig template.

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 31, 2018

Contributor

For the record, we abandon the idea of function/macros for the Grid templating in Twig because of the poor performance.

Thanks for all the tests you have done @sarjon! I'm easy with an include 👍

Contributor

mickaelandrieu commented Aug 31, 2018

For the record, we abandon the idea of function/macros for the Grid templating in Twig because of the poor performance.

Thanks for all the tests you have done @sarjon! I'm easy with an include 👍

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Aug 31, 2018

Contributor

Should be another reason why the performance sucks with Twig. Because include is a Twig function :trollface: We maybe need to investigate, otherwise we will encounter the problem in the future :/

Contributor

PierreRambaud commented Aug 31, 2018

Should be another reason why the performance sucks with Twig. Because include is a Twig function :trollface: We maybe need to investigate, otherwise we will encounter the problem in the future :/

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Aug 31, 2018

Member

@PierreRambaud includes suck only when we have fallbacks:

{% include ['very_specific.html.twig', 'specific.html', 'fallback.html.twig'] %}

I dont know why, but my guess is due to checks if template exists in filesystem.

Member

sarjon commented Aug 31, 2018

@PierreRambaud includes suck only when we have fallbacks:

{% include ['very_specific.html.twig', 'specific.html', 'fallback.html.twig'] %}

I dont know why, but my guess is due to checks if template exists in filesystem.

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 31, 2018

Contributor

Sorry for closing, we still have to add the include with ['very_specific.html.twig', 'specific.html', 'fallback.html.twig'] strategy for Grid column, grid header, and grid filter, don't you think?

I guess it's slow only in dev mode, am I right?

Contributor

mickaelandrieu commented Aug 31, 2018

Sorry for closing, we still have to add the include with ['very_specific.html.twig', 'specific.html', 'fallback.html.twig'] strategy for Grid column, grid header, and grid filter, don't you think?

I guess it's slow only in dev mode, am I right?

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Aug 31, 2018

Member

@mickaelandrieu i think it was slow in both environments (i can double check). :/

im trying to come up with another way to make templating with grid easier and more performant.

Member

sarjon commented Aug 31, 2018

@mickaelandrieu i think it was slow in both environments (i can double check). :/

im trying to come up with another way to make templating with grid easier and more performant.

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Sep 1, 2018

Contributor

From what Blackfire tells me between develop branch on your contribution, the difference of performance is ok (don't forget we make some almost static totally configurable from modules: the cost can't be 0).

blackfire with 1 entry (~ +5% diff)

blackfire with 1500 entries (~ +40% diff)

blackire with 1500 entries + my optimizations (~ -3% diff)
IMO there is an issue with the cache system as the resolution of each block should happen only once, am I right?

I end up on my portable computer to a page rendered in 124ms with 1500 entries in dev mode

worth

And 122ms in prod, I guess it's ok for a back office (at least we can still improve the performance later).

Contributor

mickaelandrieu commented Sep 1, 2018

From what Blackfire tells me between develop branch on your contribution, the difference of performance is ok (don't forget we make some almost static totally configurable from modules: the cost can't be 0).

blackfire with 1 entry (~ +5% diff)

blackfire with 1500 entries (~ +40% diff)

blackire with 1500 entries + my optimizations (~ -3% diff)
IMO there is an issue with the cache system as the resolution of each block should happen only once, am I right?

I end up on my portable computer to a page rendered in 124ms with 1500 entries in dev mode

worth

And 122ms in prod, I guess it's ok for a back office (at least we can still improve the performance later).

@mickaelandrieu mickaelandrieu changed the title from Update grid templating to Allow overriding of every part of Grid templates Sep 1, 2018

performance issues sounds solved now

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Sep 1, 2018

Member

thats nice, how about 100 records per page performance?

Member

sarjon commented Sep 1, 2018

thats nice, how about 100 records per page performance?

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Sep 1, 2018

Contributor

@sarjon I haven't tested and now I'm joining some friends to the beach so... 😎

Contributor

mickaelandrieu commented Sep 1, 2018

@sarjon I haven't tested and now I'm joining some friends to the beach so... 😎

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Sep 1, 2018

Contributor

ping @PierreRambaud are you aware of this issue on e2e tests?

sh: 1: wdio: not found

Contributor

mickaelandrieu commented Sep 1, 2018

ping @PierreRambaud are you aware of this issue on e2e tests?

sh: 1: wdio: not found

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Sep 3, 2018

Contributor

@mickaelandrieu Download can failed sometimes (don't know why), you can simple rerun tests ;)

Contributor

PierreRambaud commented Sep 3, 2018

@mickaelandrieu Download can failed sometimes (don't know why), you can simple rerun tests ;)

@mickaelandrieu mickaelandrieu added this to the 1.7.5.0 milestone Sep 6, 2018

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Sep 6, 2018

Contributor

Hi @sarjon, would you mind to cs fix again this pull request?

We need this to be shipped in 1.7.5 👍

Contributor

mickaelandrieu commented Sep 6, 2018

Hi @sarjon, would you mind to cs fix again this pull request?

We need this to be shipped in 1.7.5 👍

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon
Member

sarjon commented Sep 7, 2018

@mickaelandrieu rebased.

@matks matks added the migration label Sep 19, 2018

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Sep 20, 2018

Contributor

This needs a last review and to be tested by QA before tonight 🔥 🔥 !

Contributor

mickaelandrieu commented Sep 20, 2018

This needs a last review and to be tested by QA before tonight 🔥 🔥 !

*/
public function renderColumnContent(array $record, array $column, array $grid)
{
$templateCacheKey = sprintf('column_%s_%s_%s_content', $grid['id'], $column['id'], $column['type']);

This comment has been minimized.

@matks

matks Sep 20, 2018

Contributor

What about adding defensive validation here ?

$this->validateInput($record, $column, $grid);

private function validateInput(array $record, array $column, array $grid)
{
    $this->arrayMustContains($grid, 'id');
    $this->arrayMustContains($column, ['id', 'type]);
}

// might be an util function so could come from another class
/**
 * @param array $array
 * @param string|string[] $keys
 *
 * @throws \InvalidArgumentException
 */
protected function arrayMustContains(array $array, $keys)
{
    ...
}
@matks

matks Sep 20, 2018

Contributor

What about adding defensive validation here ?

$this->validateInput($record, $column, $grid);

private function validateInput(array $record, array $column, array $grid)
{
    $this->arrayMustContains($grid, 'id');
    $this->arrayMustContains($column, ['id', 'type]);
}

// might be an util function so could come from another class
/**
 * @param array $array
 * @param string|string[] $keys
 *
 * @throws \InvalidArgumentException
 */
protected function arrayMustContains(array $array, $keys)
{
    ...
}

This comment has been minimized.

@sarjon

sarjon Sep 20, 2018

Member

i guess we could do that, but if you pass wrong data, you will instantly that your grid is broken.

@sarjon

sarjon Sep 20, 2018

Member

i guess we could do that, but if you pass wrong data, you will instantly that your grid is broken.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 20, 2018

Contributor

I don't want the validation done directly in the class (this breaks SRP).

Note that we have complete control on grid, column, and record (see GridPresenter class).

So: we inject a Validator (or an options resolver) or we do that later, your decision @matks !

@mickaelandrieu

mickaelandrieu Sep 20, 2018

Contributor

I don't want the validation done directly in the class (this breaks SRP).

Note that we have complete control on grid, column, and record (see GridPresenter class).

So: we inject a Validator (or an options resolver) or we do that later, your decision @matks !

This comment has been minimized.

@matks

matks Sep 20, 2018

Contributor

We do that later :)

@matks

matks Sep 20, 2018

Contributor

We do that later :)

*
* @return string|null
*/
private function getTemplatePath(array $column, array $grid, $basePath, $defaultTemplate = null)

This comment has been minimized.

@matks

matks Sep 20, 2018

Contributor

This looks like a function that could be useful in other Twig extensions. Maybe we could move it into a class like GridExtensionUtils ?
Wdyt ?

@matks

matks Sep 20, 2018

Contributor

This looks like a function that could be useful in other Twig extensions. Maybe we could move it into a class like GridExtensionUtils ?
Wdyt ?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 20, 2018

Contributor

Maybe, but later: we really need this pull request to be merged today. Refactoring can happens later, as it's a private function this won't change the public API :)

@mickaelandrieu

mickaelandrieu Sep 20, 2018

Contributor

Maybe, but later: we really need this pull request to be merged today. Refactoring can happens later, as it's a private function this won't change the public API :)

This comment has been minimized.

@matks

matks Sep 20, 2018

Contributor

Agreed, let's get this merged ! :goberserk:

@matks

matks Sep 20, 2018

Contributor

Agreed, let's get this merged ! :goberserk:

return sprintf('%s/%s', $basePath, $defaultTemplate);
}
return null;

This comment has been minimized.

@matks

matks Sep 20, 2018

Contributor

@mickaelandrieu did'nt you say that you wanted that we stop returning null and throw exceptions ?

@matks

matks Sep 20, 2018

Contributor

@mickaelandrieu did'nt you say that you wanted that we stop returning null and throw exceptions ?

This comment has been minimized.

@sarjon

sarjon Sep 20, 2018

Member

it's a private function, you can see that exception is thrown above. :)

@sarjon

sarjon Sep 20, 2018

Member

it's a private function, you can see that exception is thrown above. :)

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 20, 2018

Contributor

still we could return '' or throw the exception here ahahah

@mickaelandrieu

mickaelandrieu Sep 20, 2018

Contributor

still we could return '' or throw the exception here ahahah

@matks matks merged commit 694c2a7 into PrestaShop:develop Sep 20, 2018

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@matks

matks Sep 20, 2018

Contributor

Merged as it is approved by PierreRambaud and can be improved later
QA cannot really test it as this is a backend topic not visible

Contributor

matks commented Sep 20, 2018

Merged as it is approved by PierreRambaud and can be improved later
QA cannot really test it as this is a backend topic not visible

@matks matks removed the waiting for QA label Sep 20, 2018

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