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

Catch module theme installation error, add error message for invalid module #9089

Merged
merged 4 commits into from May 28, 2018

Conversation

Projects
None yet
8 participants
@Quetzacoalt91
Member

Quetzacoalt91 commented May 18, 2018

Questions Answers
Branch? 1.7.4.x
Description?
  • If a module requested by a theme could not be installed, no error was thrown and the execution was going on. Enabling the same uninstalled module was failing and the message wasn't explicit enough.
  • A new error message has finally been added for invalid modules (missing or with a syntax error). Merchants will now get a specific error instead of the current "no more details provided" 🙌 )
Type? bug fix
Category? BO
BC breaks? Nope
Deprecations? Nope
Fixed ticket? http://forge.prestashop.com/browse/BOOM-5480
How to test? Install the theme provided in the forge issue

This change is Reviewable

@@ -590,6 +590,11 @@ public function getError($name)
if ($module->hasValidInstance()) {
$errors = $module->getInstance()->getErrors();
$message = array_pop($errors);
} else {
// Invalid instance: Missing or with syntax error
$message = $this->translator->trans('The module is invalid and cannot be loaded.',

This comment has been minimized.

@PierreRambaud

PierreRambaud May 18, 2018

Contributor

Can you split arguments across multiple lines, it's easier to read:

$message = $this->translator->trans(
    'The module is invalid and cannot be loaded.',
    [],
    'Admin.Modules.Notification'
);
'%error_details%' => $moduleManager->getError($moduleName),
),
'Admin.Modules.Notification')
);

This comment was marked as resolved.

@PierreRambaud

PierreRambaud May 18, 2018

Contributor

Same here please:

if (!$moduleManager->isInstalled($moduleName)
    && !$moduleManager->install($moduleName)
) {
    throw new PrestaShopException(
        $this->translator->trans(
            'Cannot %action% module %module%. %error_details%',
            [
                '%action%' => 'install',
                '%module%' => $moduleName,
                '%error_details%' => $moduleManager->getError($moduleName),
            ],
            'Admin.Modules.Notification'
        )
    );
}

Quetzacoalt91 added some commits May 18, 2018

@@ -348,8 +352,10 @@ public function disable($name)
if (!$this->adminModuleProvider->isAllowedAccess(__FUNCTION__, $name)) {

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 21, 2018

Contributor

sounds like this block of code is used multiple times, how about creating a reusable function?

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 May 22, 2018

Member

This is already a reused function, what do you need more?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 22, 2018

Contributor

the if block could be one function too: how about throwIfModuleAccessNotAllowed($name)?

This comment has been minimized.

@PierreRambaud

PierreRambaud May 22, 2018

Contributor

Maybe overkill, more you must pass the __FUNCTION__, $name, translated message, or message, parameters and domain. :/

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 May 22, 2018

Member

Sounds better to directly send the result of $this->translator->trans(...) in that case.

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 May 24, 2018

Member

Do you really expect another refacto in this PR?
As I'm changing some strings, I'll have to wait for 1.7.5.0 to make them available if we don't accept it for the next minor release.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 24, 2018

Contributor

nope, it's is a suggestion 👍 @PierreRambaud have forgotten to add the "waiting for QA" label

@marionf marionf added this to the 1.7.4.0 milestone May 25, 2018

@ntiepresta ntiepresta added QA ✔️ and removed waiting for QA labels May 25, 2018

@eternoendless

This comment has been minimized.

Member

eternoendless commented May 28, 2018

Thank you @Quetzacoalt91

@eternoendless eternoendless merged commit 5243d55 into PrestaShop:1.7.4.x May 28, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment