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

Minor grid improvements #10219

Merged
merged 9 commits into from Sep 3, 2018

Conversation

Projects
None yet
4 participants
@sarjon
Member

sarjon commented Aug 30, 2018

Questions Answers
Branch? develop
Description? See below.
Type? improvement
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? n/a
How to test? No behavior has changed, every grid works the same.

Changes:

  • Prefix services with Grid
  • Update hook naming & params
  • Fixes Backup grid after naming update
  • Add controller helper method presentGrid()
  • Add additional blocks for grid template

This change is Reviewable

@@ -36,7 +36,7 @@
/**
* Create new grid definition.
*
* @return DefinitionInterface
* @return GridDefinitionInterface
*/
public function getDefinition();

This comment has been minimized.

@sarjon

sarjon Aug 30, 2018

Member

these get* methods for factories still looks weird for me and gives impression of singleton pattern. :/ i think it's more common for factories to have create* or make*, @mickaelandrieu wdyt?

@sarjon

sarjon Aug 30, 2018

Member

these get* methods for factories still looks weird for me and gives impression of singleton pattern. :/ i think it's more common for factories to have create* or make*, @mickaelandrieu wdyt?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 31, 2018

Contributor

create or make doesn't bring any value for people who don't have access (or don"t read: most common) to the docs.

A factory - by design - create something: what we need to know is "what?".

@mickaelandrieu

mickaelandrieu Aug 31, 2018

Contributor

create or make doesn't bring any value for people who don't have access (or don"t read: most common) to the docs.

A factory - by design - create something: what we need to know is "what?".

This comment has been minimized.

@sarjon

sarjon Aug 31, 2018

Member

Factory says what it creates, for example FormFactoryInterface, so we know it creates forms so methods like create(), createNamed() & etc does not need to repeat it, it wont create db connection or anything unrelated. On the other hand, we could have createGrid()?

@sarjon

sarjon Aug 31, 2018

Member

Factory says what it creates, for example FormFactoryInterface, so we know it creates forms so methods like create(), createNamed() & etc does not need to repeat it, it wont create db connection or anything unrelated. On the other hand, we could have createGrid()?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 3, 2018

Contributor

Yes, but look at how we use it behind a very long service name:

what we have is most of the time $this->get('prestashop.damn_very_long_name.factory')->create(), I think it's better to have $this->get('prestashop.damn_very_long_name.factory')->getObject() because at the end of the day, it doesnt matter if we retrieve the object from factory or any creational pattern, what is important is to give meaningful information for developer who needs to manage the object :)

@mickaelandrieu

mickaelandrieu Sep 3, 2018

Contributor

Yes, but look at how we use it behind a very long service name:

what we have is most of the time $this->get('prestashop.damn_very_long_name.factory')->create(), I think it's better to have $this->get('prestashop.damn_very_long_name.factory')->getObject() because at the end of the day, it doesnt matter if we retrieve the object from factory or any creational pattern, what is important is to give meaningful information for developer who needs to manage the object :)

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 31, 2018

Contributor

Once merged, you may rebase develop because we just fixed DB backup page => #10223

Contributor

mickaelandrieu commented Aug 31, 2018

Once merged, you may rebase develop because we just fixed DB backup page => #10223

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 31, 2018

Contributor

I also agree we should provide an helper to present a Grid 👍

Contributor

mickaelandrieu commented Aug 31, 2018

I also agree we should provide an helper to present a Grid 👍

*
* @return array
*/
protected function presentGrid(GridInterface $grid)

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 31, 2018

Contributor

👍

@mickaelandrieu

mickaelandrieu Aug 31, 2018

Contributor

👍

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Sep 3, 2018

Contributor

Good for me, thanks @sarjon!

Contributor

mickaelandrieu commented Sep 3, 2018

Good for me, thanks @sarjon!

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Sep 3, 2018

Contributor

ping @marionf Grids are already covered by tests and the only bug reported is fixed here.

Contributor

mickaelandrieu commented Sep 3, 2018

ping @marionf Grids are already covered by tests and the only bug reported is fixed here.

@mickaelandrieu mickaelandrieu merged commit c325b63 into PrestaShop:develop Sep 3, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mickaelandrieu mickaelandrieu added this to the 1.7.5.0 milestone Sep 3, 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