Add feature to disable modules #340

jolelievre · 2019-09-12T16:56:48Z

This PR fixes the issue PrestaShop/PrestaShop#14779

The module can now disable custom modules by moving them into another folder
This feature was added inside the CoreUpgrader (bound to an option "Disable non native modules"
And a new controller was aded so that this new feature can be used via an API

…ble custom modules

…, add originalVersion in the state

PierreRambaud · 2019-09-13T08:03:42Z

classes/Module/ModuleRepository.php

+    public function getCustomModulesOnDisk($versions)
+    {
+        $modulesOnDisk = $this->getModulesOnDisk();
+        $nativeModules = array();


Don't mix short array and array

Right! I'm used to short array now, but in this module I think use the array() notation since we're not sure of the PHP version? wdyt?

Short array came with 5.4 and this module require 5.6+.

classes/Module/ModuleRepository.php

PierreRambaud · 2019-09-13T08:05:08Z

classes/Module/AddonsCurlClient.php

+/**
+ * Class AddonsCurlClient is a simple Addons client that uses Curl to perform its request.
+ */
+class AddonsCurlClient implements AddonsClientInterface


Is this class a copy from the Core?

Not an exact copy, but the code comes from the core yes, but I prefer to have a dedicated class to manage this call inside the module. Since we don't know on which version the module is installed we can't rely on the core classes because they are modified from a version to another.
I think that's why the Tools class was copied in Tools14

By the way I think I copied this code from the Tools class but I can't remember which version (since I was working on different versions at the moment). I also removed a few switch cases because the API simply didn't work with these parameters.

I was only asking because of snake case and some parts about services and hosted_module 😅

classes/Module/ModuleDisabler.php

classes/Module/AddonsCurlClient.php

classes/Module/AddonsClientInterface.php

classes/AutoupgradeException.php

classes/Module/AddonsCurlClient.php

PierreRambaud · 2019-09-13T08:15:56Z

classes/Module/ModuleRepository.php

+        $disabledModulesDir,
+        AddonsClientInterface $addonsClient
+    ) {
+        $this->modulesDir = rtrim($modulesDir, DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR;


Better to not use DIRECTORY_SEPARATOR constant in PHP.
If you use DIRECTORY_SEPARATOR, which resolves to \ on windows, things tend to break. For example:\ gets converted to %5C in urls.

PierreRambaud · 2019-09-13T08:17:49Z

classes/State.php

+     */
+    public function setOriginVersion($originVersion)
+    {
+        $this->originVersion = $originVersion;


Suggested change

$this->originVersion = $originVersion;

$this->originVersion = (string) $originVersion;

classes/UpgradeContainer.php

PierreRambaud · 2019-09-13T08:38:15Z

classes/UpgradeTools/ModuleAdapter.php

-        deactivate_custom_modules();
+        $customModules = $this->moduleRepository->getCustomModulesOnDisk([$this->originVersion, $this->upgradeVersion]);
+        foreach ($customModules as $moduleName) {
+            $this->moduleDisabler->disableModuleFromDatabase($moduleName);


Since this class is not final, always prefer getter function

Suggested change

$this->moduleDisabler->disableModuleFromDatabase($moduleName);

$this->getModuleDisabler()->disableModuleFromDatabase($moduleName);

In case it's overridden?

Yes, even for us we we have another Disabler based on this class. If we have setter / getter, always prefer them.

tests/ModuleRepositoryTest.php

upgrade/upgrade-5-0-0.php

jolelievre

Hi @PierreRambaud
Thanks for the review, I have a few answers/questions for you ;)

classes/AutoupgradeException.php

classes/Module/AddonsClientInterface.php

jolelievre · 2019-09-16T14:04:31Z

classes/Module/AddonsCurlClient.php

+/**
+ * Class AddonsCurlClient is a simple Addons client that uses Curl to perform its request.
+ */
+class AddonsCurlClient implements AddonsClientInterface


Not an exact copy, but the code comes from the core yes, but I prefer to have a dedicated class to manage this call inside the module. Since we don't know on which version the module is installed we can't rely on the core classes because they are modified from a version to another.
I think that's why the Tools class was copied in Tools14

jolelievre · 2019-09-16T14:05:59Z

classes/Module/AddonsCurlClient.php

+/**
+ * Class AddonsCurlClient is a simple Addons client that uses Curl to perform its request.
+ */
+class AddonsCurlClient implements AddonsClientInterface


By the way I think I copied this code from the Tools class but I can't remember which version (since I was working on different versions at the moment). I also removed a few switch cases because the API simply didn't work with these parameters.

classes/Module/ModuleRepository.php

classes/UpgradeContainer.php

jolelievre · 2019-09-16T14:17:29Z

classes/UpgradeTools/ModuleAdapter.php

-        deactivate_custom_modules();
+        $customModules = $this->moduleRepository->getCustomModulesOnDisk([$this->originVersion, $this->upgradeVersion]);
+        foreach ($customModules as $moduleName) {
+            $this->moduleDisabler->disableModuleFromDatabase($moduleName);


In case it's overridden?

tests/ModuleRepositoryTest.php

upgrade/upgrade-5-0-0.php

…installation

PierreRambaud · 2019-09-25T06:55:04Z

ping @Quetzacoalt91 Can you also have a check on it?

Progi1984 · 2020-04-09T07:34:32Z

classes/Addons/CurlClient.php

+                break;
+
+                break;


Why 2 break ?

Progi1984 · 2020-04-09T07:36:05Z

classes/Addons/CurlClient.php

+    private function parseRequest($requestContent)
+    {
+        if (empty($requestContent)) {
+            false;


Suggested change

false;

return false;

Progi1984 · 2020-04-09T07:36:21Z

classes/Exception/AutoupgradeException.php

@@ -0,0 +1,33 @@
+<?php
+/**
+ * 2007-2019 PrestaShop SA and Contributors


Please update headers

Progi1984 · 2020-04-09T07:41:44Z

classes/State.php

+     *
+     * @return self
+     */
+    public function setOriginVersion($originVersion)


Suggested change

public function setOriginVersion($originVersion)

public function setOriginVersion(string $originVersion)

We can't do this kind of changes in the autoupgrade module, it must be compatible with older PS versions :)

classes/UpgradeContainer.php

Progi1984 · 2020-06-12T14:10:55Z

Ping @jolelievre

PierreRambaud · 2020-06-12T15:51:37Z

views/templates/main.twig

+        <a target="_blank" href="{{ link.getAdminLink('AutoupgradeModule') }}&version=1.6.1">List modules 1.6.1</a>
+        <a target="_blank" href="{{ link.getAdminLink('AutoupgradeModule') }}&version=1.7.6">List modules 1.7.6</a>


Suggested change

<a target="_blank" href="{{ link.getAdminLink('AutoupgradeModule') }}&version=1.6.1">List modules 1.6.1</a>

<a target="_blank" href="{{ link.getAdminLink('AutoupgradeModule') }}&version=1.7.6">List modules 1.7.6</a>

<a target="_blank" href="{{ link.getAdminLink('AutoupgradeModule') }}&version=1.6.1">List modules 1.6.1</a>

<a target="_blank" href="{{ link.getAdminLink('AutoupgradeModule') }}&version=1.7.6">List modules 1.7.6</a>

PierreRambaud · 2020-06-12T15:52:32Z

tests/ModuleRepositoryTest.php

+    {
+        parent::setUp();
+        $this->fileSystem = new Filesystem();
+        $this->tempDir = sys_get_temp_dir() . '/module_disabler';


Just notice you should use a test directory, instead of the system directory, some container doesn't have /tmp 😅

matks · 2021-12-03T14:32:29Z

Hi @jolelievre

Since we had no news from you for more than 30 days (and some more), I will follow our guidelines and I'll close this pull request as it seems stale. Feel free to reopen or open another one if you think it's still relevant 😉

Thanks!

jolelievre added 3 commits September 12, 2019 09:50

Add new services and integrate them in a controller to enable or disa…

e0af300

…ble custom modules

Custom modules need to be computed between origin and target versions…

98535b7

…, add originalVersion in the state

Add tests and create interface for Addons client to allow testing it

7447d14