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

Disable non native modules on PS8.0 and onward #498

Merged
merged 8 commits into from
Sep 5, 2022

Conversation

matthieu-rolland
Copy link
Contributor

@matthieu-rolland matthieu-rolland commented Aug 17, 2022

Questions Answers
Description? when upgrading to PS8.0 or later, we want non native modules to be disabled
Type? improvement
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #25649
How to test? 1/ Install a non native module (for example a prestashop example module)
2/ Install autoupgrade module from this PR
3/ Remove problematic modules (welcome mbo metrics facebook psxmarketingwithgoogle)
4/ Make a release from prestashop 8.0.x with this PR
4/Upgrade your shop via local archive with your generated release
5/ After upgrade, your non native module should be disabled and you should be able to navigate the BO
Possible impacts?

@matthieu-rolland matthieu-rolland force-pushed the disable-non-native-modules branch 2 times, most recently from b0bdac7 to 9a22f9e Compare August 17, 2022 13:02
@matthieu-rolland
Copy link
Contributor Author

@PrestaShop/qa-team, @MatShir

the issue said to wait for the fix on the core before using it in the module, but the fix is only available on prestashop 8.0

So for now this PR only fixes the problem when upgrading to PrestaShop 8.0.

I think something could be done for previous PrestaShop versions, but I won't have time to fix this for previous prestashop versions before the beta.

@jolelievre jolelievre changed the title disabled non native modules on PS8.0 and onward Disable non native modules on PS8.0 and onward Aug 17, 2022
Copy link
Contributor

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

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

The modification is legit, that said I believe we already have an existing method in the module to handle this feature:

if ($this->container->getUpgradeConfiguration()->shouldDeactivateCustomModules()) {
$this->disableCustomModules();
}

Except it was bound to an option, but maybe instead of adding a new core command to execute and a new service to get from the Symfony container we could rely on existing code and either:

  • force the call of disableCustomModules in CoreUpgrader80
  • force the option shouldDeactivateCustomModules to always be true for 80

It also raises the question about the current checkbox, should it be checked and disabled by default so that the user understands that his custom modules will be disabled no matter what?

jolelievre
jolelievre previously approved these changes Aug 17, 2022
Copy link
Contributor

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

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

OK I misunderstood the purpose of the PR I though it forced disabling custom modules but no It "simply" fixes the function that handle this optional feature

All good for me

@khouloudbelguith khouloudbelguith self-assigned this Aug 17, 2022
matks
matks previously approved these changes Aug 19, 2022
Copy link

@khouloudbelguith khouloudbelguith left a comment

Choose a reason for hiding this comment

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

Hello @matthieu-rolland,

As discussed in slack, I detected this issue:

  1. Install PS1787 with English language + France country
  2. Enable mode debug + Disable cache
  3. install any non-native modules (for example Paypal, https://github.com/PrestaShop/ps_qualityassurance )
  4. Uninstall problematic modules (welcome mbo metrics facebook psxmarketingwithgoogle, checkout)
  5. Install the autoupgrade module of this PR
  6. Upgrade to the release 8.0.0 created from Branch 8.0.x
  7. See error: the upgrade is stopped

image

Untitled_.Aug.18.2022.10_15.AM.1.mp4

Thanks!

Copy link

@khouloudbelguith khouloudbelguith left a comment

Choose a reason for hiding this comment

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

Hi @matthieu-rolland,

To conclure.

Thanks!

@matthieu-rolland
Copy link
Contributor Author

thank you @khouloudbelguith for the sum up !

@matthieu-rolland
Copy link
Contributor Author

About upgrade de 1778 => 8.0.0: NOK

It will need to be tested with this PR, but with a release generated from this PR on the core

Copy link
Contributor

@jolelievre jolelievre 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 once the CI has been fixed (CS fixer, PHPStan, ...)

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.

Like @jo: the code is OK but CI needs to pass

@Robin-Fischer-PS Robin-Fischer-PS added the Waiting for review Waiting for review label Aug 31, 2022
matks
matks previously approved these changes Aug 31, 2022
@matthieu-rolland matthieu-rolland force-pushed the disable-non-native-modules branch 3 times, most recently from 9f24d72 to d537046 Compare August 31, 2022 15:33
.github/workflows/php.yml Outdated Show resolved Hide resolved
atomiix
atomiix previously approved these changes Aug 31, 2022
@jolelievre jolelievre added waiting for QA and removed Waiting for review Waiting for review labels Sep 1, 2022
@matthieu-rolland
Copy link
Contributor Author

matthieu-rolland commented Sep 1, 2022

@khouloudbelguith now it should work when upgrading from 1778, I updated the how to test with a link to the core PR, which must used to generate the prestashop release

Copy link

@khouloudbelguith khouloudbelguith left a comment

Choose a reason for hiding this comment

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

Hello @matthieu-rolland,

I am using PHP v7.2.

  • "disable non native modules enabled" : Upgrade from PS1.7.7.8 to 8.0.0 using this PR
  • Rollback from PS8.0.0 to PS1778 => ok
  • Upgrade from PS1778 => 8.0.0: Ok ( disable non-native modules enabled): before the upgrade, I used a custom theme that contains different modules non-native
  • "disable non native modules disabled" : Upgrade from PS1.7.7.8 to 8.0.0 using this PR
1778._.8.0.0.mp4

I am using PHP v7.4

  • "disable non native modules enabled" : Upgrade from PS1.7.8.7 to 8.0.0 using this PR
  • Rollback from PS8.0.0 to PS1.7.8.7 => ok
  • Upgrade from PS1.7.8.7 => 8.0.0: Ok ( disable non-native modules enabled): before the upgrade, I used a custom theme that contains different modules non-native
  • "disable non native modules disabled" : Upgrade from PS1.7.8.7 to 8.0.0 using this PR
1.7.8.7._.8.0.0.mp4

I am using PHP v7.3

  • Upgrade from PS1.7.7.8 to PS1.7.8.7 => OK
  • Rollback from PS1.7.8.7 to PS1.7.7.8 => ok

I am using PHP v7.2.

  • "disable non native modules enabled" : Upgrade from PS1.7.6.9 to 8.0.0 using this PR
  • Rollback from PS8.0.0 to PS1.7.6.9 => ok
  • Upgrade from PS1.7.6.9 => 8.0.0: Ok ( disable non-native modules enabled): before the upgrade, I used a custom theme that contains different modules non-native
  • "disable non native modules disabled" : Upgrade from PS1.7.6.9 to 8.0.0 using this PR
1.7.6.9._.8.0.0.mp4

After the upgrade
image

Thanks!

@khouloudbelguith khouloudbelguith added this to the 4.15.0 milestone Sep 5, 2022
@atomiix atomiix merged commit efe996a into PrestaShop:dev Sep 5, 2022
@atomiix
Copy link
Contributor

atomiix commented Sep 5, 2022

Well done everyone! :)

@MatShir
Copy link

MatShir commented Sep 5, 2022

yeah

atomiix added a commit to atomiix/autoupgrade that referenced this pull request Sep 16, 2022
…ative-modules

Disable non native modules on PS8.0 and onward
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants