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

Fixed the problem with the type of a return value in Module::onInstall() #11442

Merged
merged 1 commit into from Jan 28, 2019

Conversation

Projects
None yet
10 participants
@zapalm
Copy link
Contributor

zapalm commented Nov 20, 2018

Questions Answers
Branch? develop
Description? Action buttons (configure and others) of a just installed module are not displayed if the module return 1 as the success result (not a Boolean type).
Type? bug fix
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket?
How to test? Change the type of the success result from true to 1 on some module and install it.

This change is Reviewable

@prestonBot

This comment has been minimized.

Copy link
Collaborator

prestonBot commented Nov 20, 2018

Hello @zapalm!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

@Quetzacoalt91 Quetzacoalt91 added this to the 1.7.6.0 milestone Nov 20, 2018

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Nov 20, 2018

To be done in another PR:

  • Add PHP doc to mark the returned value as bool
  • Remove obsolete checks in ModuleController
@marionf

This comment has been minimized.

Copy link
Contributor

marionf commented Nov 20, 2018

Hello @zapalm
Can you provide a module allowing to test your PR ?

@zapalm

This comment has been minimized.

Copy link
Contributor Author

zapalm commented Nov 21, 2018

@mickaelandrieu
I think just to cast to Boolean in your fix is not good solution, because, seems the null type is also allowed: src/PrestaShopBundle/Controller/Admin/ModuleController.php:303
@Quetzacoalt91
I think also that to remove obsolete checks in ModuleController is not good idea.

The main question is: can the method Module::install() return also null type besides Boolean? Or why the check for non Boolean value is needed in the ModuleController?
If allowed only Boolean values then the exception should be thrown to inform the developer about the mistake.
Some examples for understanding.

public function install()
{
    if (!parent::install()) {
        return false;
    }

    // The error, because it returns null, but a module is installed correctly: forgotten `return true;`
    // So, the exception should be thrown to inform the developer about this mistake.
}

public function install()
{
    if (!parent::install()) {
        return false;
    }

    // Returns true or 1 on a success and 0 or false on an error, because of the method registerHook().
    // So, there is no mistake in general and an exception should not be thrown.
    return $this->registerHook('extraLeft');
}
@mickaelandrieu

This comment has been minimized.

Copy link
Contributor

mickaelandrieu commented Nov 21, 2018

I think just to cast to Boolean in your fix is not good solution, because, seems the null type is also allowed

For me, it shouldn't but maybe it's expected to allow use cases you suggest: WDYT @Quetzacoalt91?

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Nov 21, 2018

Modules are expected to return a boolean. True as success, false otherwise.

null is a mistake and means the module forgot to return something.

@zapalm zapalm dismissed stale reviews from Quetzacoalt91 and mickaelandrieu via 8c1637a Nov 22, 2018

@zapalm zapalm force-pushed the zapalm:develop branch 2 times, most recently from 8c1637a to a603c82 Nov 22, 2018

@zapalm

This comment has been minimized.

Copy link
Contributor Author

zapalm commented Nov 22, 2018

@Quetzacoalt91 I have added the comment to the code to describe the problem and also changed the checking a result in ModuleController.

'%action%' => ucfirst(str_replace('_', ' ', $action)),
'%module%' => $module, ),
'%module%' => $module,
'%action%' => $action,

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Nov 23, 2018

Member

There is a mistake in the original code. Would you mind using str_replace('_', ' ', $action) here too?

This comment has been minimized.

@zapalm

zapalm Nov 23, 2018

Author Contributor

Yes, no problem.

Show resolved Hide resolved src/Adapter/Module/Module.php Outdated

@zapalm zapalm force-pushed the zapalm:develop branch from a603c82 to 543f48b Nov 23, 2018

@zapalm zapalm force-pushed the zapalm:develop branch from 543f48b to 77854ac Nov 26, 2018

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Nov 28, 2018

Hi @zapalm,

I cannot update the PR for you. Would you mind rebasing it in order to fix the exiting conflicts?

Thanks

Obsolete

@zapalm zapalm force-pushed the zapalm:develop branch from 77854ac to bdcc84e Nov 28, 2018

@zapalm

This comment has been minimized.

Copy link
Contributor Author

zapalm commented Nov 28, 2018

Hi @Quetzacoalt91
I have resolved conflicts.

@Quetzacoalt91 Quetzacoalt91 force-pushed the zapalm:develop branch from 3428e80 to 2acb173 Jan 21, 2019

@mbadrani

This comment has been minimized.

Copy link
Contributor

mbadrani commented Jan 22, 2019

Cannot see this issue on develop branch before testing your PR, @zapalm can you provide me more information about how did you reproduce the issue corrected by your PR?

@zapalm

This comment has been minimized.

Copy link
Contributor Author

zapalm commented Jan 23, 2019

@mbadrani

Action buttons (configure and others) of a just installed module are not displayed if the module return 1 as the success result (not a Boolean type).

You need change the code of the install() method of a PHP class of some module, for example, like this:

public function install()
{
    if (!parent::install()) {
        return false;
    }

    return 1;
}
@mbadrani

This comment has been minimized.

Copy link
Contributor

mbadrani commented Jan 23, 2019

Hi @zapalm
I've modified the ps_wirepayment.php included in a native module ("bank transfer")
image

and after reinstalling the module (yes I've uninstalled it before and clear the cache)
image
I have everything
have you reproduced on the same module just for the example?
I am on develop branch by the way
Thanks!

@zapalm

This comment has been minimized.

Copy link
Contributor Author

zapalm commented Jan 25, 2019

@mbadrani
proof
😠

@ntiepresta ntiepresta assigned ntiepresta and unassigned ntiepresta Jan 25, 2019

@Quetzacoalt91 Quetzacoalt91 merged commit b27eece into PrestaShop:develop Jan 28, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Jan 28, 2019

Thank you @zapalm

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