-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Autoupgrade module issues with customization of SQL columns #32635
Comments
Also is it possible to execute the php upgrade scripts manually as standalone scripts ? |
@ioweb-gr Before other maintainers of the project express their words, my 5 cents. We should make those queries as modification-resistant as possible, but we can't expect what people do with the tables. Best practice is to not modify the default tables at all, custom data should be put to custom tables. However, you are doing this upgrade only once, so I think the best way is just to modify the upgrade scripts to your needs. Make a test copy of your store and just try and fix, try and fix. You will eventually make the upgrade work fine, then you can forget it. |
Hi @Hlavtox, until the next upgrade, where the same issue will come up for any additional columns. I would like to point out that even though in most cases using a separate table is good, I have to disagree for simple one-to-one mapping of rows by a separate table key to ERP id, as this would bring a penalty when we would have to join tables like clean up the data that's no longer in the ERP. As demonstrated in other discussion, we already suffer from the poor performance of the features_lang table with over 500k rows. Imagine having to join that table against an additional table and the performance hit to syncrhonize these data. Why we would need to have this field? Because they manage everything via their ERP and they have to have clear view on the shop itself of where some data is coming from. I do think that you cannot cover all cases, but these are very simple cases. In our case, 90% of the issues came from tables which aleady exist since our migration from 1.6 to 1.7 so it's not like we added them directly. That means simly adding if not exists in the queries at the very least. Even if we don't compare datatypes that's a first step. All migration tools at that time fetched the structure and the data from the previous database and replicated on the new one as the autoupgrade tool wasn't upgrading from 1.6 to 1.7. The only tools available were -> rebuild your shop -> migrate data and schema. Now we will carry this problem forever with the system making us seriously consider abandoning the platform due to that but it would be a pity for all the work we've done. In any case proper logging of the failures and the queries that failed would allows us to work through them instead of guessing. The features requested are vital for being developer-friendly and assist in the upgrade, but the bug itself should be eliminated. THe autoupgrade module can't report that you upgraded the DB, without actually upgrading it. It's totally misleading |
I agree with everything you say! If you can, it would be the best to make a PR for autoupgrade module that would bring more info on what fails. So we know what to do. Then, improve those queries one by one so they do the same things, but work with custom database columns. 👍 |
I can work on improving some of the queries that I saw causing us issues so it's a start. Would the prestashop maintainers be interested in implementing the logic of making the autoupgrade more verbose as to what it's executing and whether it's successful or not ? The ones that caused me the most issues are
The hard part for me is trying to execute the upgrade scripts from modules/autoupgrade/upgrade/php as not all of them have a version reference and other than
It's making it really hard to understand what else I need to execute to upgrade from 1.7.5.2 to 1.7.8.9 as a start. Also I'm not sure how I can execute them directly from cli even if I include them as direct scripts because the FrontController is blocking me. Any insight on that? e.g.
will yield
|
@ioweb-gr You should be able to run the scripts manually by running more info - https://devdocs.prestashop-project.org/8/basics/keeping-up-to-date/upgrade/ |
It seems that the path is wrong for the vendor libraries
I guess all it's paths need to be modified to make it work. The correct vendor dir to use is the one included in the autoupgrade module or the PS_ROOT_DIR? |
Even after fixing the paths the file require_once DIR . '/../classes/datas.php'; Does not exist. I can see it's dynamically created at
|
Even after fixing all paths
|
Hello @ioweb-gr I have updated the title of this (very interesting) discussion. I am not saying it's not a huge issue, but the title of the issues aims to
So I renamed it with that idea 😄 "Huge issue" does not help these 2 goals |
Therefore it's not possible to do this way @matks not a problem :) |
however @matks I think the title is misleading now as it doesn't gather the whole range of the issues depicted here. For example without any additional column this script will always fail
If you check here, the name of each module is not enclosed in single quotes creating an sql query like this for example Unknown column 'jscomposer' in 'where clause' UPDATE So anyone upgrading higher than version 1.7.7.0 would experience a catastrophic failure because of subsequent errors not executing. I guess that's why I found so many issues reported in the forums about this This is not related to any customization Take this also into example
This will throw an error because the query that gets executed
Doesn't get it's prefix replaced. If you notice half queries have the DB_PREFIX concatenated inthe string , and half of them have them inside the query itself. I doubt this executes correctly in any update scenario either Hence why this issue is general about the autoupgrade procedure which is a total disaster due to not having any abstraction causing these errors in the first place or a test scenario to cover them or anything to prevent someone from writing these codes |
Another example
This one can never execute in PHP context as prior to it's execution Prestashop needs to be initialized. This cannot happen because to initialize it, the currencies must be there. It's an unresolvable circular dependency basically |
OK but then what do I write 😄 ? "General problem with autoupgrade module"? The title must give a quick hint about the github issue content 😉 |
I know right? :) The problem is this is an issue with multiple sub-issues, it should be used to gather problems together in my opinion. THis way it can be safely ignored because it hints that only if you customize the columns there's an issue. In reality it's not. How about [EPIC] Autoupgrade fails to upgrade when any sql command fails to execute due to poor implementations? Then we can gather them up in one place, create sub-issues and process them in PRs |
@PrestaShop/committers could someone please try to reproduce the issue 🙏 and change the labels ;) |
The rest is not clear or result of previous failed upgrades. |
@ioweb-gr Closing this for now, if you encounter some other issues on customized shops, please let me know. We can improve it so it's more stable. :-) |
Prerequisites
Describe the bug and add attachments
We have an eshop which was migrated from prestashop 1.6 to 1.7.5.2 (latest version at that point). We're now at a spot when we try to upgrade for the past 10 months using the autoupgrade module and no matter what we tried, there are always a million database issues. To give you an example, to get to a half working state I had to execute manually queries like these
Upon deep search I noticed that all these queries are in one or more ways added by the autoupgrade module and exist in one form or the other in the folder
prestashop\autoupgrade\upgrade\php
prestashop\autoupgrade\upgrade\sql
As to why it fails, the reason is simple
For example in my case on file
1.7.6.0.sql
There is the command
But on my database the table exists and the columns are already there from the first migration I suppose.
These queries are not fault tolerant and they expect a very very predefined structure of the database making it impossible to upgrade
There are other cases like
INSERT INTO
PREFIX_hook_alias
(name
,alias
) VALUES('displayAdminOrderTop', 'displayInvoice'),
('displayAdminOrderSide', 'displayBackOfficeOrderActions'),
('actionFrontControllerInitAfter', 'actionFrontControllerAfterInit')
;
which throw a duplicate error because they should be
or delete / replace or update the values if the references exist.
There are also inserts from select which are prone to break because if a custom module has added a single column in the select table, the column count won't match and they fail to execute.
This query pre-assumes that no user has customized the columns. Well in real cases modules will add columns to tables. Users will need to add columns to tables to extend functionality, sync with ERPs etc etc.
Unless the main goal of prestashop is to never install third party modules or develop anything this will always cause issues.
And the list of course goes on and on and on and on and on.
To be honest as a last resort I started executing all the files manually one by one and realized that I could get my site to a semi-working state. However this is not all that there is, even the statements from the upgrade/php folder are not executed.
So now I have to find and execute manually all the php scripts one by one to see if I can get to a working state.
This is a very serious issue impacting a lot of users and I've been searching solutions for errors all throughout the forums pointing to the queries that need to be executed, and guess what. Every single one of them, was already in the autoupgrade module but never executed. The only logical explanation is that when an sql command fails for any reason, the autoupgrade module fails to notice it and crashes or finishes it's job ignoring them.
This is in no way proper solution and makes prestashop un-upgradeable. I can provide access to a sandbox to the prestashop team if you guys want to see what a real case scenario looks like and why this module is a total disaster for real-life situations.
In reality the module needs to be refactored to work better.
As this will always require manual work, the least it can do is point us to the problems. I have a suggestion and I would like to know if it's feasible, planned, or if there's any interest at all from the platform maintainers to address this issue.
When a script is executed there needs to be a pointer when it fails. During autoupgrade, it should mention that X script failed to execute successfully whether sql or php. Ideally we would need to know the exact line which caused the error. All commands in the sql / php scripts could be sequentially executed and output possible errors in execution so that the final result we can get a list of failed queries that we can adjust manually. This can be direct output or an error.log
Separate schema changes from data changes. If the schema is not correct you shouldn't migrate data. Trying to execute an insert while the add column statement failed because it's duplicate may not always work as the column data type might be different than expected.
Additional columns in the schema unless specifically deleted on purpose during an upgrade script, should be left untouched and ignored as it could result in data loss
Insert queries should explicitly set the columns to insert values to and not rely on the columns existing on the table because their count might be different than the select column count.
Alter table statements which add columns should use an
add column if not exists
statement instead to bypass other issues with previous executions of the autoupgrade module or previous migrations or other cases where the column already exists in the table for one reason or the other.There needs to be schema proper schema comparison for each column description compared to the final target schema. For example it could point out that the schema expects table column X to be unsigned int(10) but it's unsinged int
These are all issues I faced during upgrade that I'm suggesting should be fixed to provide a seemless upgrade.
I come from a Magento 2 background and I'll tell you that they solved this by abstracting table definition in XML files and now all newest version XML files are parsed, and the ORM handles the migrations of the columns. During setup:db-schema:upgrade, among other things the expected schema is collected as an abstract definition and the ORM verifies it against the existing schema. It decides what columns need to be updated, created or deleted based on the schema definition.
Migration to Doctrine should have solved this issue because it would make it much more simple to abstract everything.
Expected behavior
Autoupgrade doesn't break as easily.
Steps to reproduce
PrestaShop version(s) where the bug happened
1.7.5.2 1.7.8.9
PHP version(s) where the bug happened
7.3 7.4
If your bug is related to a module, specify its name and its version
autoupgrade latest version from repo
Your company or customer's name goes here (if applicable).
IOWEB TECHNOLOGIES
The text was updated successfully, but these errors were encountered: