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

MailTheme example module for PS8 #122

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

0x346e3730
Copy link
Member

@0x346e3730 0x346e3730 commented Nov 8, 2022

Questions Answers
Description? Upgrade the MailTheme example module for PS8. Needs PrestaShop/PrestaShop#30295.
Type? refacto
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #108.
How to test? Try the module and make sure it's working as intended.
Possible impacts? mailtheme module only.

Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

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

Nice work bro

@0x346e3730
Copy link
Member Author

Try to modify getTemplatePath

@0x346e3730 0x346e3730 changed the title WIP MailTheme example module for PS8 MailTheme example module for PS8 Nov 15, 2022
@0x346e3730
Copy link
Member Author

Also needs PrestaShop/PrestaShop#30295

Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

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

Ok for me, let's wait for PrestaShop/PrestaShop#30295 to be merged

@@ -1,4 +1,7 @@
{% extends '@PrestaShop/Admin/layout.html.twig' %}
{% extends '@!PrestaShop/Admin/layout.html.twig' %}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't remember, what is the interest of the initial ! ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's to be sure we use PrestaShop's template and not one from a module as it could lead to a infinite loop

@0x346e3730
Copy link
Member Author

0x346e3730 commented Dec 16, 2022

The new constructor of the FolderThemeScanner class in this PR breaks this PR because of $scanner = new FolderThemeScanner(); which is no longer working because it's missing the module directory.

Edit : Fixed https://github.com/PrestaShop/example-modules/pull/122/files#diff-249c27b04b40dc32437d71beb17091c58b095da676a26a085bca04adfd1dc35eR230 but this PR now needs PrestaShop/PrestaShop#30295 to be merged first

@AureRita
Copy link

Hi @0x346e3730,

I tested your module with the PR #30295 and currently we have some issue on it. as you can see :

Untitled_.Dec.16.2022.11_44.AM.webm

@matks
Copy link
Contributor

matks commented Jan 13, 2023

Hello @AureRita I am afraid the bug you observe is not related to the PR and the initial topic of the subject #108.

I understand that it does prevent you from testing the PR, but @0x346e3730 cannot start working on #108 then on another bug PrestaShop/PrestaShop#30290 then on another found during QA testing, it's never going to end ^^ . The PR is going to grow, it will be more complex, will need git rebase and so on.

So I discussed with @0x346e3730 and we pause this PR for now, he cannot continue this work as we have no idea when the list of bugs to fix will end. We don't know when (if) he will be able to dedicate time to this PR again.

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.

Make example modules works with Prestashop 8.0
5 participants