Skip to content
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

Copy demoextendsymfonyform1 into demoextendsymfonyform3 #33

Merged
merged 2 commits into from
Jan 21, 2021

Conversation

matks
Copy link
Contributor

@matks matks commented Jan 18, 2021

Questions Answers
Description? There is confusion about Demo Symfony Form module 1 because the module demonstrates 2 things: how to use a CQRS in a module AND how to use hooks to add new field to a Symfony form

Some community developers think that the two must be together although actually you can choose to use CQRS or not to add new field to a Symfony form.

To fix this I will split this module into 2 demo modules. This PR creates the new module.
Type? improvement
BC breaks? no
Deprecations? no
Fixed ticket? Fixes 1 item of #32
How to test? No need to test
Possible impacts?

This modules demonstrates
- how to add this field, manage its content and its
properties using modern hooks in Symfony pages
- how to use custom [CQRS](https://devdocs.prestashop.com/1.7/development/architecture/domain/cqrs/) Commands and Queries to separate your domain from your application
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- how to use custom [CQRS](https://devdocs.prestashop.com/1.7/development/architecture/domain/cqrs/) Commands and Queries to separate your domain from your application
- how to use custom [CQRS](https://devdocs.prestashop.com/1.7/development/architecture/domain/cqrs/) Commands and Queries to separate your domain from your application *

- `composer install` - to download dependencies into vendor folder
4. Install module from Back Office

*Because the name of the directory and the name of the main module file must match.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
*Because the name of the directory and the name of the main module file must match.
**Because the name of the directory and the name of the main module file must match.**


parent::__construct();

$this->displayName = $this->getTranslator()->trans(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$this->displayName = $this->getTranslator()->trans(
$this->displayName = $this->trans(

no need to get translator here, there's shorter trans method in Module

);
}

throw new \PrestaShop\PrestaShop\Core\Module\Exception\ModuleErrorException($message);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not importing it?

*
* @return RedirectResponse
*/
public function toggleIsAllowedForReviewAction($customerId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public function toggleIsAllowedForReviewAction($customerId)
public function toggleIsAllowedForReviewAction(int $customerId)

Copy link
Contributor

@kpodemski kpodemski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feedback

@matks
Copy link
Contributor Author

matks commented Jan 21, 2021

Hi @kpodemski as this PR is a simple copy can I keep the improvements for a later PR ? 😊 If it's a 100% identical copy, the great thing is that we are sure "it works as before" whereas if we introduce modifications, we now need to re-validate it entirely.

/**
* Class DemoExtendSymfonyForm1 demonstrates the usage of CQRS pattern and hooks.
*/
class DemoExtendSymfonyForm3 extends Module
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as here #35 (comment)

I think it's a good thing to split into two modules to make it less confusing, but we really need to improve the naming of these two module Or it will remain confusing and they won't meet the expected objective

Suggested names for this one:

  • DemoExtendSymfonyPageWithCQRS
  • DemoExtendWithCQRS
  • DemoCQRSPattern
  • DemoCQRSForExtension

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jolelievre I'll perform the renaming with the improvements suggested by @kpodemski in a 2nd PR to keep it scoped 😉 and I created an issue for global renaming #40

@kpodemski
Copy link
Contributor

No problem @matks 👍

@matks
Copy link
Contributor Author

matks commented Jan 21, 2021

Thank you for review 😉

@matks matks merged commit 8fd3d4f into PrestaShop:master Jan 21, 2021
@matks matks deleted the split-module-1 branch January 21, 2021 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants