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

Migrate Performance page to Symfony #8279

Merged
merged 21 commits into from Oct 24, 2017

Conversation

Projects
None yet
9 participants
@mickaelandrieu
Contributor

mickaelandrieu commented Aug 28, 2017

Questions Answers
Branch? develop
Description? Migration of Performance page to Symfony
Type? improvement
Category? CO
BC breaks? yes, hooks displayAdminForm and displayAdminPerformanceFormhooks are not available.
Deprecations? no
Fixed ticket? http://forge.prestashop.com/browse/BOGOSS-43
How to test? Access Configure > Advanced Parameters > Performances

actual_status

Todo

  • Form validation messages can't be translated with a custom domain (Symfony limitation)
  • Re implement Memcache servers management (REST API)
  • Re implement Memcache servers management (Vue javascript front)
  • Re add Demo mode restrictions
  • Re implement permissions
  • Re implement Form javascript hide/show/animations
  • Complete POST processing
  • Re implement Hooks (Depends on #8327)
  • Re implement description message in forms
  • Don't forget to implement FeatureInterface on *Feature classes
  • Remove old controller and template
  • Register new route
  • Removed not used translation hints message on select options and hint with same message as labels...

Note on hooks:
displayAdminForm and displayAdminPerformance haven't been re-introduced as they are totally dependent to legacy forms, and won't work as expected in Symfony pages.
The right way to override a form right now is to override the related service in a module. The right way to override a form view is to override the related Twig block in a module. See #8342 for more information.


This change is Reviewable

@PrestaShop PrestaShop deleted a comment from prestonBot Aug 29, 2017

@mickaelandrieu mickaelandrieu added the WIP label Aug 29, 2017

{
$builder
->add('smart_cache_css', 'Symfony\Component\Form\Extension\Core\Type\ChoiceType', array(
'choices' => array(

This comment has been minimized.

@aleeks

aleeks Aug 30, 2017

Contributor
 'choices'  => array(
    0 => $this->translator->trans('No', [], 'Admin.Global'),
    1 => $this->translator->trans('Yes', [], 'Admin.Global'),
),

or it's translated with the resolver ? (if we have some new strings, they aren't send to crowdin with this current code)

@aleeks

aleeks Aug 30, 2017

Contributor
 'choices'  => array(
    0 => $this->translator->trans('No', [], 'Admin.Global'),
    1 => $this->translator->trans('Yes', [], 'Admin.Global'),
),

or it's translated with the resolver ? (if we have some new strings, they aren't send to crowdin with this current code)

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 30, 2017

Contributor

it will be translated using choice_translation_domain when needed (which is the case here, just didn't finished to add this property everywhere. You don't need to include the translator in a form type, never!

@mickaelandrieu

mickaelandrieu Aug 30, 2017

Contributor

it will be translated using choice_translation_domain when needed (which is the case here, just didn't finished to add this property everywhere. You don't need to include the translator in a form type, never!

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 30, 2017

Contributor

just noticed our form template was deprecated (the migration may have started with Symfony 2.7 imo...), so I've updated the template according to the latest one from Symfony: thanks for your hint :)

updatedtranslations

@mickaelandrieu

mickaelandrieu Aug 30, 2017

Contributor

just noticed our form template was deprecated (the migration may have started with Symfony 2.7 imo...), so I've updated the template according to the latest one from Symfony: thanks for your hint :)

updatedtranslations

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 30, 2017

Contributor

For Crowdin, it is indeed an issue, we may implement a new rule in translation tool because we also use translation domain rule in twig template (as @eternoendless asked) so I guess theses keys won't be re-imported on Crowdin too, am I right?

@mickaelandrieu

mickaelandrieu Aug 30, 2017

Contributor

For Crowdin, it is indeed an issue, we may implement a new rule in translation tool because we also use translation domain rule in twig template (as @eternoendless asked) so I guess theses keys won't be re-imported on Crowdin too, am I right?

This comment has been minimized.

@AlexEven

AlexEven Oct 5, 2017

Contributor

Can you please explain? ^^;

@AlexEven

AlexEven Oct 5, 2017

Contributor

Can you please explain? ^^;

value="{{ choice.value }}" {{ block('attributes') }}{% if choice is selectedchoice(value) %} selected="selected"{% endif %}>{{ choice_translation_domain is same as(false) ? choice.label : choice.label }}</option>
{%- endif -%}
{% endfor %}
{% for group_label, choice in options %}

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 31, 2017

Contributor

for the record: allow the use of choice_translation_domain in Form types.

@mickaelandrieu

mickaelandrieu Aug 31, 2017

Contributor

for the record: allow the use of choice_translation_domain in Form types.

return $this->redirectToRoute('admin_performance');
}

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 31, 2017

Contributor

dispatch Hook here.

@mickaelandrieu

mickaelandrieu Aug 31, 2017

Contributor

dispatch Hook here.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 1, 2017

Contributor

@aleeks what do you think of the data sent to the hook? should we send something more useful instead?

@mickaelandrieu

mickaelandrieu Sep 1, 2017

Contributor

@aleeks what do you think of the data sent to the hook? should we send something more useful instead?

* This feature is not available in Symfony so we need to inject the translator
* for constraints messages only.
*/
abstract class TranslatorAwareType extends AbstractType

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 1, 2017

Contributor

you only need to inherit from this service to make the translator service available in every FormType class

@mickaelandrieu

mickaelandrieu Sep 1, 2017

Contributor

you only need to inherit from this service to make the translator service available in every FormType class

@@ -321,7 +321,7 @@
{% endif %}
<label{% for attrname, attrvalue in label_attr %} {{ attrname }}="{{ attrvalue }}"{% endfor %}>
{{- widget|raw -}}
{{- label is not same as(false) ? (translation_domain is same as(false) ? label : label) -}}
{{- label is not same as(false) ? (translation_domain is same as(false) ? label|raw : label|raw) -}}

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 1, 2017

Contributor

As Symfony forms are an internal feature, it's not a security issue (this block generates the option label block of a form).

@mickaelandrieu

mickaelandrieu Sep 1, 2017

Contributor

As Symfony forms are an internal feature, it's not a security issue (this block generates the option label block of a form).

Tools::clearSmartyCache();
Tools::clearXMLCache();
Media::clearCache();
Tools::generateIndex();

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Sep 1, 2017

Member

With this call we are not cleaning the cache.

Don't you think we should have a CacheWarmer somewhere?

@Quetzacoalt91

Quetzacoalt91 Sep 1, 2017

Member

With this call we are not cleaning the cache.

Don't you think we should have a CacheWarmer somewhere?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 1, 2017

Contributor

Hmm at this point, this is not expected to clear the Symfony cache here. We should probably use CacheWarmer, not in the adapter but in the corresponding "new" class (this need a refactoring of how PrestaShop manages to clear his cache(s), it's a really big topic :) )

@mickaelandrieu

mickaelandrieu Sep 1, 2017

Contributor

Hmm at this point, this is not expected to clear the Symfony cache here. We should probably use CacheWarmer, not in the adapter but in the corresponding "new" class (this need a refactoring of how PrestaShop manages to clear his cache(s), it's a really big topic :) )

if (!empty($serverOne) || !empty($serverTwo) || !empty($serverThree)) {
$this->configuration->set('PS_MEDIA_SERVERS', 1);
} else {

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 1, 2017

Contributor

we have found a bot more annoying than @prestonBot :trollface:

@mickaelandrieu

mickaelandrieu Sep 1, 2017

Contributor

we have found a bot more annoying than @prestonBot :trollface:

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 11, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 11, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 11, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 11, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 11, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 11, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 11, 2017

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Oct 14, 2017

Contributor

@Quetzacoalt91 I've fixed the error: you can't access add/test server actions only if you select Memcache cache option.

Contributor

mickaelandrieu commented Oct 14, 2017

@Quetzacoalt91 I've fixed the error: you can't access add/test server actions only if you select Memcache cache option.

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 14, 2017

@Quetzacoalt91 Quetzacoalt91 added this to the 1.7.4.0 milestone Oct 16, 2017

@Quetzacoalt91

Some lines to change, but without any impact of the code execution, and a big to do list to write.

Let's get this PR checked by the QA team

@marionf

This comment has been minimized.

Show comment
Hide comment
@marionf

marionf Oct 16, 2017

Contributor

Hi,

If I disable the features or the combinations, there is a link on these pages to the Performance page.
pr8279 1

It's the old link, so the page can't be found
pr8279 2

Contributor

marionf commented Oct 16, 2017

Hi,

If I disable the features or the combinations, there is a link on these pages to the Performance page.
pr8279 1

It's the old link, so the page can't be found
pr8279 2

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Oct 19, 2017

Contributor

@marionf well spotted, I'll fix asap and let you know 👍

edit: can you confirm you didn't found any other bug? Let's be efficient ^^

Contributor

mickaelandrieu commented Oct 19, 2017

@marionf well spotted, I'll fix asap and let you know 👍

edit: can you confirm you didn't found any other bug? Let's be efficient ^^

@marionf marionf added QA ✔️ and removed waiting for QA labels Oct 23, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 23, 2017

@Quetzacoalt91 Quetzacoalt91 merged commit 09a488b into PrestaShop:develop Oct 24, 2017

2 of 3 checks passed

code-review/reviewable 55 files left
Details
codacy/pr Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mickaelandrieu mickaelandrieu deleted the mickaelandrieu:migration/performance-page branch Oct 24, 2017

@matks matks added the migration label Sep 18, 2018

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