thirtybees / coreupdater

thirty bees module for updating thirty bees core.
Academic Free License v3.0
0 stars 6 forks source link

Database list of differences is a minefield #29

Open musicpanda opened 1 year ago

musicpanda commented 1 year ago

After an upgrade that crashed halfways I had no other alternative than using the list of database differences. Only too late I discovered that while I was madly clicking those about 100 buttons I had clicked too much and deleted extra fields that belong to my modules and theme. I wasted several hours due to this.

The "fix" is taking the position that everything extra is suspect unless you know that it belongs to a module. That is the opposite of how it should be. Almost nobody knows which fields belong to a module. All the extra's should be in a separate table on a different page so that they can not be accidentally clicked. This should apply both to extra fields and fields that are bigger than the Thirty Bees default. That should also apply to fields that are now TEXT instead of VARCHAR(255).

Thirty Bees is adding fields like crazy. In that light it is astonishing that it considers every field added by a module as suspect.

When imposing indexes Thirty Bees should respect extra fields. My update failed because the shop uses Attribute Wizard Pro (AWP) and AWP uses a fifth field in the primary key of tb_cart_product. The Thirty Bees upgrade bluntly imposes a primary key with that extra field and then the upgrade crashed because the key is no longer unique.

getdatakick commented 1 year ago

I understood your rant as an UX improvement request -- hide some classes of database differences. That makes sense.

Other notes:

In that light it is astonishing that it considers every field added by a module as suspect.

Well, of course we mark them as suspect. Extra columns cause a lot of troubles. Especially if they are not nullable and/or are part of indexes. Core code (or third party modules code) does not know about existence of these columns, so INSERT statements don't mention them, which can lead to database exception. Of course, author of those modules 'fixes' this by overriding various core methods, but that is very brittle solution -- core code refactoring have tendencies to breaks this. Module authors should never change database objects they don't own.

When imposing indexes Thirty Bees should respect extra fields. My update failed because the shop uses Attribute Wizard Pro (AWP) and AWP uses a fifth field in the primary key of tb_cart_product. The Thirty Bees upgrade bluntly imposes a primary key with that extra field and then the upgrade crashed because the key is no longer unique.

Core updater creates indexes as they are specified in object model metadata. Core code depends on indexes behaviour, especially on unique indexes. If this behaviour changes, core code stops working.

musicpanda commented 1 year ago

Core updater creates indexes as they are specified in object model metadata. Core code depends on indexes behaviour, especially on unique indexes. If this behaviour changes, core code stops working.

Its flexibility and extensibility is what makes Prestashop a great system. I understand that that sometimes means extra work when maintaining the code. But without it you have a much poorer system.

getdatakick commented 1 year ago

Its flexibility and extensibility is what makes Prestashop a great system. I understand that that sometimes means extra work when maintaining the code. But without it you have a much poorer system.

True.

But it's still extendable, though. Module author can inform core about their changes. Module can override object model definition, and add their own custom fields and indexes into it. Core updater would pick these overrides and would honor them. Core code will know that new fields exist, will know about types of those columns and use explicit or implicit default values when creating new records, etc.

If module does not tell us about their changes to OUR table, it can't be surprised when something goes wrong.

musicpanda commented 1 year ago

If module does not tell us about their changes to OUR table, it can't be surprised when something goes wrong.

The strength of Thirty Bees is that it is a continuation of Prestashop 1.6. You are slowly turning it in something completely different that is incompatible with Prestashop 1.6.

Most of the bugs I run into when I try to upgrade or migrate a shop are nowadays "improvements" that you unilaterally added. In the name of stability you create a minefield.

musicpanda commented 1 year ago