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

Use official Smarty lib #8566

Merged
merged 3 commits into from Dec 13, 2017

Conversation

Projects
None yet
4 participants
@Quetzacoalt91
Member

Quetzacoalt91 commented Nov 30, 2017

Questions Answers
Branch? develop
Description? This PR brings back smarty/smarty into the composer.json, allowing PrestaShop to get the latest changes. This PR goes with the PHP 7.2 compatibility and PrestaShop/TranslationToolsBundle#39
Type? improvement
Category? CO
BC breaks? Nope
Deprecations? Nope
Fixed ticket? /
How to test? I see 3 main things to check:
  • Theme export, via cli or back-office
  • Testing the basic features (already covered by the tests?)
  • Testing the child/parent themes feature.

This change is Reviewable

@Quetzacoalt91 Quetzacoalt91 changed the title from Smarty to Use official Smarty lib Nov 30, 2017

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Nov 30, 2017

Contributor

I don't have rights on TranslationToolsBundle, can someone review and merge the related pr?

Contributor

mickaelandrieu commented Nov 30, 2017

I don't have rights on TranslationToolsBundle, can someone review and merge the related pr?

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Nov 30, 2017

Contributor

@Quetzacoalt91 how the QA team should test it? I'm not really sure :/

Contributor

mickaelandrieu commented Nov 30, 2017

@Quetzacoalt91 how the QA team should test it? I'm not really sure :/

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Dec 1, 2017

Member

I see 3 main things to check:

  • Theme export, via cli or back-office
  • Testing the basic features (already covered by the tests?)
  • Testing the child/parent themes feature.
Member

Quetzacoalt91 commented Dec 1, 2017

I see 3 main things to check:

  • Theme export, via cli or back-office
  • Testing the basic features (already covered by the tests?)
  • Testing the child/parent themes feature.
@marionf

This comment has been minimized.

Show comment
Hide comment
@marionf

marionf Dec 4, 2017

Contributor

@Quetzacoalt91
I have an exception when I try to clear the cache in BO
capture du 2017-12-04 18-07-38

Contributor

marionf commented Dec 4, 2017

@Quetzacoalt91
I have an exception when I try to clear the cache in BO
capture du 2017-12-04 18-07-38

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Dec 5, 2017

Member

ping @mickaelandrieu

@marionf, this issue is actually unrelated to the PR. I came back to the branch develop, and requested the translation bundle before the merge of its dependency:

diff --git i/composer.json w/composer.json
index a33a7ba26a..9ce7627458 100644
--- i/composer.json
+++ w/composer.json
@@ -25,7 +25,7 @@
         "symfony/monolog-bundle": "^3.1.0",
         "sensio/distribution-bundle": "^5.0.19",
         "sensio/framework-extra-bundle": "^3.0.2",
-        "prestashop/translationtools-bundle": "dev-master",
+        "prestashop/translationtools-bundle": "dev-master#736c93cfd815015083e54010090bc33fd1585734",
         "tecnickcom/tcpdf": "6.2.12",
         "composer/installers": "1.0.21",
         "icanboogie/cldr": "1.3.9",

but the issue still occurs.

It seems the kernel is not aware yet of the new service container during the script exection. Making it fail when we try to update the translation catalog with the old container.

Please go on the review. I'll try to fix it at the same time.

Member

Quetzacoalt91 commented Dec 5, 2017

ping @mickaelandrieu

@marionf, this issue is actually unrelated to the PR. I came back to the branch develop, and requested the translation bundle before the merge of its dependency:

diff --git i/composer.json w/composer.json
index a33a7ba26a..9ce7627458 100644
--- i/composer.json
+++ w/composer.json
@@ -25,7 +25,7 @@
         "symfony/monolog-bundle": "^3.1.0",
         "sensio/distribution-bundle": "^5.0.19",
         "sensio/framework-extra-bundle": "^3.0.2",
-        "prestashop/translationtools-bundle": "dev-master",
+        "prestashop/translationtools-bundle": "dev-master#736c93cfd815015083e54010090bc33fd1585734",
         "tecnickcom/tcpdf": "6.2.12",
         "composer/installers": "1.0.21",
         "icanboogie/cldr": "1.3.9",

but the issue still occurs.

It seems the kernel is not aware yet of the new service container during the script exection. Making it fail when we try to update the translation catalog with the old container.

Please go on the review. I'll try to fix it at the same time.

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Dec 5, 2017

Member

I also have the issue encountered on #8515, regarding

Call to undefined method Doctrine\Bundle\DoctrineCacheBundle\Command\ContainsCommand::getDefaultName()

Could be related to the dropped cache as well.

Member

Quetzacoalt91 commented Dec 5, 2017

I also have the issue encountered on #8515, regarding

Call to undefined method Doctrine\Bundle\DoctrineCacheBundle\Command\ContainsCommand::getDefaultName()

Could be related to the dropped cache as well.

presteus added a commit to presteus/PrestaShop that referenced this pull request Dec 6, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Dec 6, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Dec 7, 2017

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Dec 7, 2017

Member

@marionf, that issue was actually linked to that PR. Cannot explain in details, but I had to get rid of Symfony 3.4 on this branch.
If Travis check switches to green, you may go on. :)

Member

Quetzacoalt91 commented Dec 7, 2017

@marionf, that issue was actually linked to that PR. Cannot explain in details, but I had to get rid of Symfony 3.4 on this branch.
If Travis check switches to green, you may go on. :)

@PrestaShop PrestaShop deleted a comment from codacy-bot Dec 7, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Dec 7, 2017

Quetzacoalt91 added some commits Dec 7, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Dec 7, 2017

@mickaelandrieu mickaelandrieu merged commit 7a90eae into PrestaShop:develop Dec 13, 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
@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu
Contributor

mickaelandrieu commented Dec 13, 2017

Thank you @Quetzacoalt91

@Quetzacoalt91 Quetzacoalt91 deleted the Quetzacoalt91:smarty branch Dec 13, 2017

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