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

Comply to validator.prestashop.com rules #43

Merged
merged 4 commits into from Jul 26, 2019

Conversation

@matks
Copy link

commented Jun 18, 2018

This is a PR to comply with Modules validation standards (validator.prestashop.com).

Before the PR, there uses to be 4 errors and 63 warnings.
The PR lowers these numbers to 49 warnings.

I did not fix it all because the last errors/warnings do not seem relevant to me. This is mostly "line exceeds 120 characters" warnings that, if fixed, would decrease the code readability.

@MrBaiame

This comment has been minimized.

Copy link

commented Jun 20, 2018

Looks good to me, since it's for a module, I join the talk. I don't know if I have the right to merge the pull request though ? @eternoendless

@MrBaiame MrBaiame closed this Jun 20, 2018

@MrBaiame MrBaiame reopened this Jun 20, 2018

$table_name = _DB_PREFIX_.bqSQL(self::$module->name);
$select_query = sprintf(
"SELECT * FROM `%s` WHERE `id_shop` = '%s' AND `id_shop_group` = '%s'",

This comment has been minimized.

Copy link
@eternoendless

eternoendless Jul 9, 2018

Member

You should use %d instead of %s for ints, don't you think?

-`id_shop` = '%s'
+`id_shop` = %d
-`id_shop_group` = '%s'
+`id_shop_group` = %d

This comment has been minimized.

Copy link
@matks

matks Jul 10, 2018

Author

Fixed in 17c3dfe

config.xml Outdated
@@ -2,7 +2,7 @@
<module>
<name>cronjobs</name>
<displayName><![CDATA[Cron tasks manager]]></displayName>
<version><![CDATA[1.3.2]]></version>
<version><![CDATA[1.4.0]]></version>

This comment has been minimized.

Copy link
@eternoendless

eternoendless Jul 9, 2018

Member

It's best to leave the version bump to the moment we merge on master

This comment has been minimized.

Copy link
@matks

matks Jul 10, 2018

Author

Fixed in 17c3dfe

if (!defined('_PS_VERSION_')) {
exit;
}
function upgrade_module_1_0_6($module)
{
if (Db::getInstance()->ExecuteS('SHOW COLUMNS FROM `'._DB_PREFIX_.$module->name.'` LIKE \'one_shot\'') == false) {
Db::getInstance()->Execute('ALTER TABLE `'._DB_PREFIX_.$module->name.'` ADD `one_shot` BOOLEAN NOT NULL DEFAULT 0 AFTER `updated_at`');
$table_name = _DB_PREFIX_ . $module->name;

This comment has been minimized.

Copy link
@eternoendless

eternoendless Jul 9, 2018

Member

Shouldn't $module->name be escaped?

This comment has been minimized.

Copy link
@matks

matks Jul 10, 2018

Author

Fixed in 17c3dfe

@matks matks force-pushed the matks:clean branch from 17c3dfe to ba92a06 Jul 10, 2018

@matks

This comment has been minimized.

Copy link
Author

commented Jul 10, 2018

@eternoendless All requested changes have been handled

@sarahdib sarahdib added QA approved and removed Waiting for QA labels Jul 8, 2019

@PierreRambaud PierreRambaud merged commit bd6c064 into PrestaShop:dev Jul 26, 2019

@matks matks deleted the matks:clean branch Jul 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.