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

Make Symfony form management more robust #8561

Merged
merged 1 commit into from Dec 11, 2017

Conversation

Projects
None yet
4 participants
@mickaelandrieu
Contributor

mickaelandrieu commented Nov 28, 2017

Questions Answers
Branch? develop
Description? Added new interfaces, making our own implementations more robusts.
Type? improvement
Category? CO
BC breaks? yes (you can't extend Performance Data provider/form handler anymore)
Deprecations? yes
How to test? Nothing to test

Important guidelines


This change is Reviewable

@mickaelandrieu mickaelandrieu added this to the 1.7.4.0 milestone Nov 28, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Nov 28, 2017

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Nov 29, 2017

Member

Just one question, why are these classes store in the Core folder (and not in PrestaShopBundle)?

Member

Quetzacoalt91 commented Nov 29, 2017

Just one question, why are these classes store in the Core folder (and not in PrestaShopBundle)?

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Nov 29, 2017

Contributor

@Quetzacoalt91 we should keep in bundle only classes related to the integration of PrestaShop in Symfony:

  • sf forms
  • sf controllers
  • sf commands
  • sf compiler pass
  • sf service declarations
  • sf framework configuration
  • Twig templates

If someday we want to re-use theses "Core classes" into a specific package it will be easier to do it.

If I could do it, I'd move the Translation folder into Core, but it's too late... (and it's my fault ;) )

Contributor

mickaelandrieu commented Nov 29, 2017

@Quetzacoalt91 we should keep in bundle only classes related to the integration of PrestaShop in Symfony:

  • sf forms
  • sf controllers
  • sf commands
  • sf compiler pass
  • sf service declarations
  • sf framework configuration
  • Twig templates

If someday we want to re-use theses "Core classes" into a specific package it will be easier to do it.

If I could do it, I'd move the Translation folder into Core, but it's too late... (and it's my fault ;) )

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Nov 29, 2017

Member

I heard the Core was supposed to disappear. 🤔

Member

Quetzacoalt91 commented Nov 29, 2017

I heard the Core was supposed to disappear. 🤔

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Nov 29, 2017

Contributor

I heard the Core was supposed to disappear. 🤔

Nope, it's Adapter namespace that will be removed when the migration will be done, for now we keep the class that depends both on legacy and modern sides in Adapter namespace 👍.

We should write the migration strategy somewhere, and I now exactly where ^o^

=> Can you merge it please ? :)

Contributor

mickaelandrieu commented Nov 29, 2017

I heard the Core was supposed to disappear. 🤔

Nope, it's Adapter namespace that will be removed when the migration will be done, for now we keep the class that depends both on legacy and modern sides in Adapter namespace 👍.

We should write the migration strategy somewhere, and I now exactly where ^o^

=> Can you merge it please ? :)

@kpodemski

This comment has been minimized.

Show comment
Hide comment
@kpodemski

kpodemski Nov 29, 2017

Contributor

I think that we all can agree that Core/Adapter/PrestaShopBundle is messy right now, some stuff in Adapter shouldn't be there, for example stuff related to products listing is in both places at the same time which is not good

Contributor

kpodemski commented Nov 29, 2017

I think that we all can agree that Core/Adapter/PrestaShopBundle is messy right now, some stuff in Adapter shouldn't be there, for example stuff related to products listing is in both places at the same time which is not good

@codacy-bot

This comment has been minimized.

Show comment
Hide comment
@codacy-bot

codacy-bot Nov 30, 2017

Codacy Here is an overview of what got changed by this pull request:

Complexity decreasing per file
==============================
+ src/PrestaShopBundle/Form/Admin/AdvancedParameters/Administration/FormHandler.php  -2
+ src/PrestaShopBundle/Form/Admin/AdvancedParameters/Administration/FormDataProvider.php  -2
+ src/PrestaShopBundle/Form/Admin/AdvancedParameters/Performance/PerformanceFormDataProvider.php  -2
+ src/PrestaShopBundle/Form/Admin/AdvancedParameters/Performance/PerformanceFormHandler.php  -2
         

See the complete overview on Codacy

codacy-bot commented Nov 30, 2017

Codacy Here is an overview of what got changed by this pull request:

Complexity decreasing per file
==============================
+ src/PrestaShopBundle/Form/Admin/AdvancedParameters/Administration/FormHandler.php  -2
+ src/PrestaShopBundle/Form/Admin/AdvancedParameters/Administration/FormDataProvider.php  -2
+ src/PrestaShopBundle/Form/Admin/AdvancedParameters/Performance/PerformanceFormDataProvider.php  -2
+ src/PrestaShopBundle/Form/Admin/AdvancedParameters/Performance/PerformanceFormHandler.php  -2
         

See the complete overview on Codacy

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Dec 11, 2017

Contributor

ping? could be nice to merge it, we are talking about adding only 1 interface "FormDataProviderInterface".

Contributor

mickaelandrieu commented Dec 11, 2017

ping? could be nice to merge it, we are talking about adding only 1 interface "FormDataProviderInterface".

@mickaelandrieu mickaelandrieu referenced this pull request Dec 11, 2017

Merged

Migrate Logs page to Symfony #8523

19 of 19 tasks complete

@Quetzacoalt91 Quetzacoalt91 merged commit 71ce2ab into PrestaShop:develop Dec 11, 2017

2 checks passed

Codacy/PR Quality Review Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment