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

Refactored the Grid component #9470

Merged
merged 4 commits into from Aug 24, 2018

Conversation

Projects
None yet
5 participants
@mickaelandrieu
Contributor

mickaelandrieu commented Aug 21, 2018

Questions Answers
Branch? develop
Description? Some parts are not consistents in terms of naming + missing Hooks
Type? improvements
Category? CO
BC breaks? no
Deprecations? no
How to test? Logs page should works, see PrestaShop/docs#116 for details.

This change is Reviewable

@sarjon

I may join to help you with update if needed. 👍

/**
* @param ColumnCollectionInterface $columns
*/
public function setColumns($columns)

This comment has been minimized.

@sarjon

sarjon Aug 22, 2018

Member

are you sure we need these setters?

  1. they are not part of contract
  2. you can do columns and actions modification using$definition->getColumns()->add(..add column) or $definition->getColumns()->remove(...column to remove)
@sarjon

sarjon Aug 22, 2018

Member

are you sure we need these setters?

  1. they are not part of contract
  2. you can do columns and actions modification using$definition->getColumns()->add(..add column) or $definition->getColumns()->remove(...column to remove)

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

I don't need them to be part of the contract as we retrieve a Definition instance and not a DefinitionInterface.

We need at least setName, isn't it? And people will expect setColumns() for sure

@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

I don't need them to be part of the contract as we retrieve a Definition instance and not a DefinitionInterface.

We need at least setName, isn't it? And people will expect setColumns() for sure

This comment has been minimized.

@sarjon

sarjon Aug 22, 2018

Member

but shouldnt Definition be final as it implements DefinitionInterface?

Regarding setName(), i guess you should be able to change it, but with other setter, i dont know.. :/
Definition already has ColumnCollection set (in constructor when created), there is no need to be able to set it again, because if you want to change something in it, you can do using $definition->getColumns().

@sarjon

sarjon Aug 22, 2018

Member

but shouldnt Definition be final as it implements DefinitionInterface?

Regarding setName(), i guess you should be able to change it, but with other setter, i dont know.. :/
Definition already has ColumnCollection set (in constructor when created), there is no need to be able to set it again, because if you want to change something in it, you can do using $definition->getColumns().

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

ok so the module and the docs should describe that: I keep the setter for the name.

Also, there is no link between implementing an interface and being final. The way I see it:

  • implementing the interface means you have access to methods available into the interface, you'll see that in the end the DefinitionFactory create a concrete implementation of Definition class :) This is why it's ok to have a setter into our concrete implementation that is not part of contract because we never assume we have this setter on something that expect the interface.

  • it is true that I'd use to make my final classes implement an interface to let developers use their own implementations: we're doing an extensible CMS and I'm removing overrides system

  • for me, making my class final means "this class have and only have the behavior we expect for it, if you want to alter it, rely on the contract and not on our own"

this is really defensive programming, I try to stabilize a 10 years old software and I need to ensure that what we provide can't be broken :)

@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

ok so the module and the docs should describe that: I keep the setter for the name.

Also, there is no link between implementing an interface and being final. The way I see it:

  • implementing the interface means you have access to methods available into the interface, you'll see that in the end the DefinitionFactory create a concrete implementation of Definition class :) This is why it's ok to have a setter into our concrete implementation that is not part of contract because we never assume we have this setter on something that expect the interface.

  • it is true that I'd use to make my final classes implement an interface to let developers use their own implementations: we're doing an extensible CMS and I'm removing overrides system

  • for me, making my class final means "this class have and only have the behavior we expect for it, if you want to alter it, rely on the contract and not on our own"

this is really defensive programming, I try to stabilize a 10 years old software and I need to ensure that what we provide can't be broken :)

/**
* {@inheritdoc}
*/
final public function create()
final public function getDefinition()

This comment has been minimized.

@sarjon

sarjon Aug 22, 2018

Member

hmm, isnt create() or createNew() a better name? It is a factory, it's responsibility is to create new instances every time you ask for it, getDefinition() is weird imo. 🤔

@sarjon

sarjon Aug 22, 2018

Member

hmm, isnt create() or createNew() a better name? It is a factory, it's responsibility is to create new instances every time you ask for it, getDefinition() is weird imo. 🤔

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

no, create or createNew isn't giving me any information on what I retrieve.

@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

no, create or createNew isn't giving me any information on what I retrieve.

This comment has been minimized.

@sarjon

sarjon Aug 22, 2018

Member

$productsDefinition = $productsGridDefinitionFactory->createNew() it gives information when variable naming is correct :) as getDefinition() does not give information whether we get new or same instance every time (my opinion).

@sarjon

sarjon Aug 22, 2018

Member

$productsDefinition = $productsGridDefinitionFactory->createNew() it gives information when variable naming is correct :) as getDefinition() does not give information whether we get new or same instance every time (my opinion).

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

it's a factory: what do you expect to retrieve from a factory? 👼

@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

it's a factory: what do you expect to retrieve from a factory? 👼

}
/**
* {@inheritdoc}
*/
public function createUsingSearchCriteria(SearchCriteriaInterface $searchCriteria)
public function getGrid(SearchCriteriaInterface $searchCriteria)

This comment has been minimized.

@sarjon

sarjon Aug 22, 2018

Member

same here with naming, are you getting or creating new grid. What's more, we should not repeat Grid in method names is guess?

@sarjon

sarjon Aug 22, 2018

Member

same here with naming, are you getting or creating new grid. What's more, we should not repeat Grid in method names is guess?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

it's consistent naming:

  • getDefinition() : DefinitionInterface
  • getGrid() : GridInterface
  • ...

this way people can't be wrong on what they expect to get using these methods, even if there have no access to docs.

@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

it's consistent naming:

  • getDefinition() : DefinitionInterface
  • getGrid() : GridInterface
  • ...

this way people can't be wrong on what they expect to get using these methods, even if there have no access to docs.

Show outdated Hide outdated src/Core/Grid/GridFactory.php
Show outdated Hide outdated src/Core/Grid/GridFactory.php
Show outdated Hide outdated src/Core/Grid/GridFactory.php
@@ -97,7 +97,7 @@ public function indexAction(Request $request, EmailLogsFilter $filters)
public function searchAction(Request $request)
{
$definitionFactory = $this->get('prestashop.core.grid.definition.factory.email_logs');
$emailLogsDefinition = $definitionFactory->create();
$emailLogsDefinition = $definitionFactory->getDefinition();

This comment has been minimized.

@sarjon

sarjon Aug 22, 2018

Member

since hook GridDefinitionModifier were move from DefinitionFactory to GridFactory we have an issue here.

Imagine some module updated grid definition with new filter/column, the changes wont be reflected here, but when presenting grid, they would be as hooks are dispatched in Presenter only.
That's why I wanted to keep hooks in factories to make sure that each time you create new instance, hook is dispatched and you have final instance.

@sarjon

sarjon Aug 22, 2018

Member

since hook GridDefinitionModifier were move from DefinitionFactory to GridFactory we have an issue here.

Imagine some module updated grid definition with new filter/column, the changes wont be reflected here, but when presenting grid, they would be as hooks are dispatched in Presenter only.
That's why I wanted to keep hooks in factories to make sure that each time you create new instance, hook is dispatched and you have final instance.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

not really, it's because we're doing object construction in controllers when we shouldn't do it: we should rely on the grid directly instead of rebuilding the form from grid definition :/

in all cases, depending on hook dispatcher everywhere is a terrible idea: it's like having access to the context

@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

not really, it's because we're doing object construction in controllers when we shouldn't do it: we should rely on the grid directly instead of rebuilding the form from grid definition :/

in all cases, depending on hook dispatcher everywhere is a terrible idea: it's like having access to the context

This comment has been minimized.

@sarjon

sarjon Aug 22, 2018

Member

it's because we're doing object construction in controllers when we shouldn't do

agree, but if we were to create whole grid, we would need to do unnecessary SQL queries even though db data is not used here, only filter form is important. :/

At the end:

“There are no solutions. There are only trade-offs.”
― Thomas Sowell

@sarjon

sarjon Aug 22, 2018

Member

it's because we're doing object construction in controllers when we shouldn't do

agree, but if we were to create whole grid, we would need to do unnecessary SQL queries even though db data is not used here, only filter form is important. :/

At the end:

“There are no solutions. There are only trade-offs.”
― Thomas Sowell

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

As I can't find (right now) a better way to do this, I change but I'm not satisfied :(

@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

As I can't find (right now) a better way to do this, I change but I'm not satisfied :(

@@ -1,3 +1,5 @@
parameters:

This comment has been minimized.

@sarjon

sarjon Aug 22, 2018

Member

at some point this file could be split into several files, like grid/data_factory.yml, grid/definition_factory.yml & etc. Similar as we did with forms in bundle, as there will be quite a few service definitions regarding grid, wdyt? :)

@sarjon

sarjon Aug 22, 2018

Member

at some point this file could be split into several files, like grid/data_factory.yml, grid/definition_factory.yml & etc. Similar as we did with forms in bundle, as there will be quite a few service definitions regarding grid, wdyt? :)

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

I agree, but looking at services I'm pretty sure we can experiment auto configuration instead: don't you think?

@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

I agree, but looking at services I'm pretty sure we can experiment auto configuration instead: don't you think?

This comment has been minimized.

@sarjon

sarjon Aug 22, 2018

Member

you mean autowire?

@sarjon

sarjon Aug 22, 2018

Member

you mean autowire?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

both!

@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

both!

use Exception;
class InvalidDataException extends Exception implements ExceptionInterface

This comment has been minimized.

@sarjon

sarjon Aug 22, 2018

Member

i guess we could have base GridException extends Exception implements ExceptionInterface and instead class InvalidDataException extends GridException? other exceptions would need to be updated too.

@sarjon

sarjon Aug 22, 2018

Member

i guess we could have base GridException extends Exception implements ExceptionInterface and instead class InvalidDataException extends GridException? other exceptions would need to be updated too.

This comment has been minimized.

@PierreRambaud

PierreRambaud Aug 24, 2018

Contributor

Agreed with @sarjon 👍

@PierreRambaud

PierreRambaud Aug 24, 2018

Contributor

Agreed with @sarjon 👍

Show outdated Hide outdated src/Core/Grid/Presenter/GridPresenter.php
@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Aug 22, 2018

Member

@mickaelandrieu regarding component update, i have few ideas :)

  1. Grid/Collection/AbstractCollection i think we can either get rid of it or move somewhere else, as it is very generic class and can be used anywhere in core.
  2. Actions that are under Grid/Actions namespace could be unified maybe? There are 3 groups of actions: grid, bulk & row. Could we keep one ActionInterface instead of 3 different?
Member

sarjon commented Aug 22, 2018

@mickaelandrieu regarding component update, i have few ideas :)

  1. Grid/Collection/AbstractCollection i think we can either get rid of it or move somewhere else, as it is very generic class and can be used anywhere in core.
  2. Actions that are under Grid/Actions namespace could be unified maybe? There are 3 groups of actions: grid, bulk & row. Could we keep one ActionInterface instead of 3 different?
@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

Grid/Collection/AbstractCollection i think we can either get rid of it or move somewhere else, as it is very generic class and can be used anywhere in core.

And this will make the component dependent of the core => meh

Actions that are under Grid/Actions namespace could be unified maybe? There are 3 groups of actions: grid, bulk & row. Could we keep one ActionInterface instead of 3 different?

Don't get caught into this trap! Every use case can evolve and have his own lifecycle, from a domain point of view, Grid actions, Bulk actions and Row actions are different things

Contributor

mickaelandrieu commented Aug 22, 2018

Grid/Collection/AbstractCollection i think we can either get rid of it or move somewhere else, as it is very generic class and can be used anywhere in core.

And this will make the component dependent of the core => meh

Actions that are under Grid/Actions namespace could be unified maybe? There are 3 groups of actions: grid, bulk & row. Could we keep one ActionInterface instead of 3 different?

Don't get caught into this trap! Every use case can evolve and have his own lifecycle, from a domain point of view, Grid actions, Bulk actions and Row actions are different things

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Aug 22, 2018

Member

And this will make the component dependent of the core => meh

then lets get rid of it? 😃 it is not really that useful (as it seemed before) and not even sure methods provided by this abstract collection are used at all.

Member

sarjon commented Aug 22, 2018

And this will make the component dependent of the core => meh

then lets get rid of it? 😃 it is not really that useful (as it seemed before) and not even sure methods provided by this abstract collection are used at all.

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

then lets get rid of it?

we need the collections to be countable and iterable and I'm pretty sure we will add some helpers like filter(), map(), something you can retrieve in Laravel Collections for instance :)

But I want the community to ask for it before introduce the features

Contributor

mickaelandrieu commented Aug 22, 2018

then lets get rid of it?

we need the collections to be countable and iterable and I'm pretty sure we will add some helpers like filter(), map(), something you can retrieve in Laravel Collections for instance :)

But I want the community to ask for it before introduce the features

@PierreRambaud

@sarjon Is it ok for you?

@PierreRambaud PierreRambaud added this to the 1.7.5.0 milestone Aug 24, 2018

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 24, 2018

Contributor

@sarjon we'll address the AbstractException question if we need to add more exceptions, the scope of this pull request was to be compliant with docs about Hooks and naming.

But now it's merged, you can rebase your pull request about data providers factories improvements ;)

Contributor

mickaelandrieu commented Aug 24, 2018

@sarjon we'll address the AbstractException question if we need to add more exceptions, the scope of this pull request was to be compliant with docs about Hooks and naming.

But now it's merged, you can rebase your pull request about data providers factories improvements ;)

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 24, 2018

Contributor

ping @eternoendless this don't needs QA, in case of error, I'll fix it.

Contributor

mickaelandrieu commented Aug 24, 2018

ping @eternoendless this don't needs QA, in case of error, I'll fix it.

@mickaelandrieu mickaelandrieu merged commit c389519 into PrestaShop:develop Aug 24, 2018

1 check passed

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

@mickaelandrieu mickaelandrieu deleted the mickaelandrieu:grid/docs-compliancy branch Aug 24, 2018

@matks matks added the migration label Sep 19, 2018

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