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.

Sign up for GitHub

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

Jump to bottom

Fix 1.6 -> 1.7 migration ; add a few class stubs #6531

Merged
merged 1 commit into from Oct 20, 2016
Merged

Fix 1.6 -> 1.7 migration ; add a few class stubs #6531

merged 1 commit into from Oct 20, 2016

Conversation

jocel1
Copy link
Contributor

@jocel1 jocel1 commented Sep 29, 2016 •
edited

Questions Answers
Branch? develop
Description? Move a few Install files into PrestaShopBundle/Install; Add stubs for core classes & add an Adapter namespace around core classes; Properly create sf database based on entities, properly migrate the admin theme
Type? bug fix
Category? IN
BC breaks? no
Deprecations? no
Fixed ticket? Could have a positive impact on BOOM-1451 (not tested yet)
How to test? Install a 1.6.1.x shop, then upgrade to 1.7.x codebase, and run upgrade.php

julienbourdeau
julienbourdeau reviewed Sep 29, 2016
View reviewed changes
install-dev/upgrade/upgrade.php
eval('abstract class AbstractLogger extends AbstractLoggerCore{}');
require_once(_PS_INSTALL_PATH_.'upgrade/classes/FileLogger.php');
eval('class FileLogger extends FileLoggerCore{}');

$cacheDir = _PS_ROOT_DIR_.'/'.(_PS_MODE_DEV_ ? 'dev' : 'prod').'/log/';
@mkdir($cacheDir, 0777, true);
$logger = new FileLogger();
Copy link
Contributor

Choose a reason for hiding this comment

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

What is FileLogger now? I dont see any use and it seems deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was already loaded in classes/log/AbstractLogger.php, same for FileLoggerCore.php

classes/Language.php
@@ -882,6 +882,8 @@ public static function checkAndAddLanguage($iso_code, $lang_pack = false, $only_
}
}

self::loadLanguages();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why you add this? I guess there is a good reason but I don't get it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you add a new language through checkAndAddLanguage, it will not update the local cache. Hence If you're using getLanguages() just after adding a language with checkAndAddLanguage(), the new language will not be returned.

@jocel1 jocel1 force-pushed the hotfix-upgrade-2 branch 2 times, most recently from e4285c9 to 8a5202f Compare October 12, 2016 12:17
Shudrum
Shudrum reviewed Oct 18, 2016
View reviewed changes
install-dev/controllers/console/process.php
*/
public function processNextStep()
{
}

public function display()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this empty method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it now implements HttpConfigureInterface and the function display() is mandatory

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, ok, so the interface is the problem, but not the subject of this PR.

Shudrum
Shudrum reviewed Oct 18, 2016
View reviewed changes
install-dev/upgrade/sql/1.7.0.0.sql

ALTER TABLE `PREFIX_tab` ADD `icon` varchar(32) DEFAULT '';

/* PHP:migrate_tabs_17(); */;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please be careful of the new lines :neckbeard:

Shudrum
Shudrum reviewed Oct 18, 2016
View reviewed changes
install-dev/upgrade/upgrade.php
@@ -34,6 +34,60 @@
$engineType = 'ENGINE_TYPE';
define('PS_IN_UPGRADE', 1);

$incompatibleModules = [
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that this array should be here…

Right now it might works, but only for 1.6 to 1.7. If for the 1.8 we re-add a module named … blockcms for example, this module will be on the ignored list.

This script, I think, is called only one time when you upgrade.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first I wanted to be able to fetch the information from the addons api, but that's not easy. That's why I finally choose to put a hardcoded list of modules. But we could fix it properly later, once it'll be easy to fetch the compatibility info from the addons api, based on the module name.

Shudrum
Shudrum reviewed Oct 18, 2016
View reviewed changes
install-dev/upgrade/upgrade.php
@@ -66,6 +120,9 @@

require_once(INSTALL_PATH . 'install_version.php');

// remove old unsupported classes
@unlink(__DIR__.'/../../../classes/db/MySQL.php');
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait … The Autoupgrade module doesn't remove the files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add least the manual execution of upgrade.php doesn't :)

@aleeks
Copy link
Contributor

aleeks commented Oct 18, 2016 •
edited

Hello @jocel1
I make some comments on this PR #6692 about sql.
Some problem with date default value to '0000-00-00' what do you think ?
Same on db_structure, really make sense to have it ?

Moreover, I install an 1.6.1.7, make the 1.7.0.0 sql script, and I had some differences, I don't know if it's normal, but just to say.

Present in 1.7 and not in 1.6.1.7 with 1.7.0.0.sql

  • ps_admin_filter
  • ps_email_subscription
  • ps_link_block
  • ps_link_block_lang
  • ps_link_block_shop
  • ps_reassurence
  • ps_reassurence_lang
  • ps_translation

In 1.6.1.7 with 1.7.0.0.sql but not in 1.7.0.0 clean install

  • ps_advise

  • ps_advise_lang

  • ps_badge

  • ps_badge_lang

  • ps_cms_block

  • ps_cms_block_lang

  • ps_cms_block_page

  • ps_cms_block_shop

  • ps_condition

  • ps_condition_advise

  • ps_condition_badge

  • ps_cronjobs

  • ps_layered_friendly_url

  • ps_newsletter

  • ps_tab_advise

  • ps_theme_configurator

    we can talk on gtalk if you want.

@jocel1
Copy link
Contributor Author

jocel1 commented Oct 18, 2016

I think the missing tables are fixed by this PR : basically it's all the tables created by doctrine from symfony entities, and which wasn't properly handled by upgrade.php

@aleeks aleeks mentioned this pull request Oct 19, 2016
Merged
@jocel1
CO : Move a few Install files into PrestaShopBundle/Install
a19a802 
CO : Add stubs for core classes & add an Adapter namespace around core classes
IN : Properly create sf database based on entities, properly migrate the admin theme
@julienbourdeau
Copy link
Contributor

Red build is a false alert. I checked locally.

Thanks Jocelyn!

@julienbourdeau julienbourdeau merged commit 8a1ebaa into PrestaShop:develop Oct 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julienbourdeau julienbourdeau julienbourdeau left review comments

@Shudrum Shudrum Shudrum left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jocel1 @aleeks @julienbourdeau @Shudrum