magento / magento2

Prior to making any Submission(s), you must sign an Adobe Contributor License Agreement, available here at: https://opensource.adobe.com/cla.html. All Submissions you make to Adobe Inc. and its affiliates, assigns and subsidiaries (collectively “Adobe”) are subject to the terms of the Adobe Contributor License Agreement.
http://www.magento.com
Open Software License 3.0
11.56k stars 9.32k forks source link

(Slightly Selfish) Request: Uninstall and downgrade scripts. #395

Closed clockworkgeek closed 9 years ago

clockworkgeek commented 11 years ago

As a third party dev I sometimes make changes to the database which I should like to reverse were my extension ever be uninstalled. This feature has been requested for several years but now with version 2.0 there is an opportunity. Any previous concerns you may have had with backward compatibility don't apply now.

For example I recently added an attribute with a custom backend model. When the extension is removed so is the backend model but the attribute remains and causes an error on every page request.

As well as an uninstall script to reverse the action of an install script I would also like to use a sequence of downgrades to complement each upgrade step. That way I don't have to write a monolithic uninstall script for every version.

antonmakarenko commented 11 years ago

@clockworkgeek With regards to example you provided about backend model -- that's actually not a DB schema change -- that's undoing a "data fixture" (like app/code/Magento/Cms/data/cms_setup/data-install-1.6.0.0.php)

Could you clarify, if you are requesting schema or data uninstaller, or both?

In your example, what if the reference to backend model was not present in DB in the first place -- would you still need that attribute to be deleted from DB? If the attribute stayed, but wouldn't cause an error on every page request, would you (or user) care for deleting it? I'm not trying to dismiss your request -- just trying to understand it better.

lukenm commented 11 years ago

:+1 @clockworkgeek This would be a great feature. Another example for when this would be useful is when working on multiple branches in development, particularly in a multi-dev team. Switching to a branch sometimes runs an install script for a module that may introduce a DB change that breaks when switching back to master. It would be great to be able to run an uninstall script that reverses the changes before switching back to master. In some cases it would be helpful to be able to roll back to a particular version of a module, rather than doing a complete uninstall.

aligentjim commented 11 years ago

:+1: Removing (or disabling) extensions which provide Backend Models and Custom Sources for attributes is an issue. @antonmakarenko yes, if a missing backend model or source didn't cause a fatal error that would resolve this situation, but having a full roll-forwards/roll-backwards migration system would help to address a number of other common development scenarios as @lukenm describes.

Vedrillan commented 11 years ago

I think this is a very good request x)

A long time ago I noticed the lack of rollback capability into the setup and upgrade process, and I also noticed that some code is present for that matter but is not fully implemented and as a result is not used.

This is not only a matter of uninstalling, but a serious necessity for delivery process. When a you deliver a new version of your modules, you need to be able to rollback in case something goes not as expected, but currently to achieve that you need dump or custom scripts, and this way is out of the Magento upgrade process.

Also, with the current Magento setup process you can only go forward, so if an upgrade went wrong, to fix it you need a new upgrade script to fix the previous one, and it's not logical to upgrade again to revert a change, because instead of going back to safety you again take the risk of having new issues x)

In any case, having the possibility to write our rollback scripts would be a good thing. Same goes for uninstall scripts.

aligentjim commented 11 years ago

Here's a concrete example of working across multiple branches. Feature X has been deployed into production. while version 2 of Feature X is under active development and introduces a number of database schema changes. If I need to make bug fixes on the original version, I'd like to "roll back" the schema changes introduced in the new version. Make a bug fix on the original version (on another branch), then allow the upgrade scripts to run again next time I switch to the version 2 branch.

We're dealing with that scenario on a project right now, and with no roll back scripts, it usually means making schema changes by hand.

clockworkgeek commented 11 years ago

@antonmakarenko You are absolutely right, in my example I was thinking of data versions. However it is possible to make schema changes which are incompatible with earlier versions/other extensions. It is my understanding the same method is followed for both "data" and "sql" processes, since I'm making a wish I choose to have both please.

@lukenm The exact same thing happens to me but I have learned workarounds. Sometimes I have several databases and include the app/etc/local.xml file in the branch so when I make a switch the database is switched too. I do have to be careful not to let my local.xml get carried upstream, an automatic downgrade path would be convenient here.

@Vedrillan Rolling back to specific versions would be great, especially if the downloader supported retrieval of any prior version. However I'm hesitant to call it a justification for downgrades. When installing extensions you should never do it on a production site, especially without making a backup first. The downloader has an option to automate that so there is no excuse. When working on a client's site I always make a local copy for testing and only allow extensions to be installed there first. Again, being able to rollback would be more convenient than restoring from backup.

Vedrillan commented 11 years ago

I never said I deliver modules directly on production x)

But when you deliver all your changes of your modules to production, even if you tested you delivery package on pre-production, Murphy would agree you that there is always a chance for something to go wrong, and when that happens, reimporting the backup made before the delivery and reverting sources is not a good solution in my opinion (even if it's working and absolutely reliable).

A quick way to revert the module(s) upgrades/setup generating issues would be nice, it would prevent a full rollback and the process of testing again the changes, the delivery and etc and etc

kalenjordan commented 11 years ago

:thumbsdown:

Since Magento 2 is already behind schedule, I say that we avoid adding complex features to their list. This could make a great hackathon project though!

antonmakarenko commented 11 years ago

@Vedrillan @clockworkgeek How far do you want to go: do you need imperative scripts that would say system "what to do" (similar to what we have for existing setup scripts) or some sort of declarative form "what I want" (it is conceptually completely different from existing setup architecture).

Assuming sticking to the current concepts (imperative scripts), is the script going to make assumptions about existing state of DB schema (or data)? Is there any kind of conditional interleaving necessary?

For example:

Or your Foo extension v3 would just delete user data from v2 -- and that would be the essence of uninstall script?

Would that script require dependency only at the Catalog schema v3 (because it may be irrelevant for another schema)? Maybe I'm stretching with this example -- I'd appreciate if you elaborated on your own.

clockworkgeek commented 11 years ago

do you need imperative scripts that would say system "what to do"

The imperative form is much easier to implement at this stage. I imagine attempting a declarative schema system would be close to a complete rewrite!

Or your Foo extension v3 would just delete user data from v2 -- and that would be the essence of uninstall script?

In that example v3 is another upgrade. A downgrade is something significantly different where the actions are reversed; data is collated back into foo_product_tax and then drop foo_product_website_tax so that if the same upgrade is executed a second time the state of the database (or files maybe) is as expected.

For a schema related use case consider that extension Foo adds a column to an existing table. Extension Bar uses that same table in it's model, the table is queried with SELECT * FROM sales_flat_quote and because the model extends Varien_Object the extra column becomes a value in bar's model. That was unintended and perhaps causes a bug, until a fix is devised the best thing is to rollback Foo.

Assuming sticking to the current concepts (imperative scripts), is the script going to make assumptions about existing state of DB schema (or data)?

If a module had previously executed install-1.0.php and then upgrade-1.0-1.1.php it can only assume the DB state is as it was at version 1.1. The uninstall process would then have to call downgrade-1.1-1.0.php followed by uninstall-1.0.php. It really is a reversal of the current method.


@kalenjordan I'd welcome a little volunteer work if I thought Magento Inc. were supportive. They've already asked for contributions.

antonmakarenko commented 11 years ago

@clockworkgeek Thank you for elaborating on the details -- it it will allow Magento product team understand intent better.

choukalos commented 11 years ago

When uninstalling would you want the ability to save extension specific customer data?

When uninstalling an extension that's been upgraded several times ( say version 1.3 ) - and the install/downgrade scripts are per version change - would you want/should there just be 1 uninstall.php that handles the extension uninstall or something else?

What's more important rollback or uninstall?

benmarks commented 11 years ago

Whoa, everyone say hi to the product manager! /waves

clockworkgeek commented 11 years ago

Hi, the product manager!

I would prefer not to destroy any data whenever possible. The various examples brought up in this issue show how sometimes leaving data intact actually puts the application in a damaged state, removing the problem (e.g. custom attributes with custom frontend/backend models) is the only solution.

I would expect a sequence of upgrade scripts to be mirrored by identically numbered downgrade scripts. Whenever another version is published a matching upgrade and downgrade step is written without having to change any of the prior steps. This makes rollbacks possible which is nice to have. However uninstall is more important, I consider that one feature to be critical.

Thank you for listening.

piotrekkaminski commented 11 years ago

Do you guys have any good example how this is solved in another system? I think staged downgrade scripts is going to be a problem (easy to make a mistake). What if in version 1 you created a table in version 2 removed it and moved data to two separate tables. Now the uninstall script would recreate the table from version 1? Move data back?

lukenm commented 11 years ago

@piotrekkaminski I haven't had a lot of experience with this, but some of the tools I have seen before are Propel (http://propelorm.org/documentation/09-migrations.html) and DbDeploy (http://dbdeploy.com/).

aligentjim commented 11 years ago

Laravel migrations have both an "up()" method for upgrades and a "down()" method for rollbacks. It's up to the author of the migration how they choose to handle a rollback (i.e. whether to drop a table or keep it "just in case"). The artisan command line tool in Laravel handles running the necessary migrations in the appropriate sequence.

See: http://laravelbook.com/laravel-migrations-managing-databases/ and http://laravel.com/docs/migrations for more info.

jonpday commented 11 years ago

I've had excellent experiences with the two-way migrations in Laravel :+1: I think the obligation should be on the developer to write safe (idempotent?) upgrade and downgrade scripts. I would not feel comfortable with Magento (or any other app) trying to interpolate a downgrade path.

orlangur commented 11 years ago

@piotrekkaminski it's easy with Doctrine Migrations.

What if in version 1 you created a table in version 2 removed it and moved data to two separate tables. Now the uninstall script would recreate the table from version 1? Move data back?

It's named not uninstall but rollback - the set of operations exactly opposite to upgrade. So it can collect data from two tables and put to the old one.

@jonpday

I think the obligation should be on the developer to write safe (idempotent?) upgrade and downgrade scripts

The fact that rollbacks actually roll the data and structure back must be controlled by continuous integration.

piotrekkaminski commented 11 years ago

@orlangur Are you taking into account large DB with tons of data? Rolling it back one by one could take days. CI usually verifies stuff on small data set (so you get results fast). Correct me if i'm wrong.

kalenjordan commented 11 years ago

Okay I'm gonna go ahead and start to build out a simple uninstall command for n98-magerun. It's not going to tackle anything complicated like the issues that have been raised in this thread.

Probably just parse out the various files the module created (layout, template, skin, etc.), and also drop all the db tables.

Just mentioning it here in case anyone else is already aware of such a project, so that I don't reinvent the wheel. I'm aware of MageTrashApp, but it requires the developer to write a SQL uninstall script which isn't really what I'm after.

orlangur commented 11 years ago

@piotrekkaminski rollbacks in Doctrine has nothing to do with tons of data. As contrary to rollback of transaction, which may be quite heavy, it's just a symmetric operation.

If DB allows you to perform upgrade, the rollback is possible to be processed as well. There is no real difference, your rollback script may be even an upgrade for some other scenario.

CI I mentioned would allow just to control operations symmetry. For load testing other environment should be prepared which reflects the real-world's one.

aoldoni commented 10 years ago

:+1: useful for proper module downgrade handling by the developer.

waynetheisinger commented 10 years ago

@piotrekkaminski wordpress has always rolled back database changes on removing plugins, though not on downgrading them- http://bit.ly/1khxe5O - as you probably know wordpress uses the concept of hooks http://codex.wordpress.org/Plugin_API and apparently has

register_activation_hook register_deactivation_hook register_uninstall_hook

Could Magento 2 do the same with Events??

And @kalenjordan if we are simply talking about raising three additional events, and letting module developers actually decide what to do when those events triggered then I don't think it would be huge amount of work and I don't think it would be adding complex features to the core team's list. However I do think you are correct if we consider the more difficult task of downgrading - which is running before we can walk perhaps - as a store owner my biggest issue with modules is the lack of a true un-install and for that matter a true disactivate, I've seldom wanted to downgrade modules (in fact I've never needed to), but I've wanted to uninstall or disactivate them on multiple occasions.

waynetheisinger commented 10 years ago

When I was at Magento live in the UK @Vinai, asked in response to my above point, "how can an observer fire if the module files have been deleted."

So to clarify my idea - magento doesn't delete the files, it instead despatches the register_uninstall event or whatever the event is called, the module writer would have written the module to then deactivate (accept for the class which handles the uninstall) this class in the module then does the uninstallation on the database as programmed by the module provider and as its final action it despatches a confirm_database_uninstall event, magento then responds to this event by deleting the files.

The way I see it, if Mage2 handles uninstallation this way the advantage is that if a module provider hasn't thought how to uninstall a module, they wouldn't have written a class to respond to the register_uninstall event, and therefore wouldn't despatch the confirm_database_uninstall event and no files would be deleted, which is no worse situation than we are in now.

Though hopefully most would write the class, especially if MagentoConnect made an uninstall a requirement, or if they baulked at doing so, at least visually made it VERY obvious when an uninstall class was included in the module, this could be at three different levels

Uninstalls: Yes (verified by Author) Uninstalls: No (claimed by the community) Uninstalls: Yes (verified by Magento)

The above would save work for Magento staff - it would be a self certification, which if users complained about would change to No, and if the Module Author wished to regain their uninstalls status could be escalated to Magento staff member.

Vinai commented 10 years ago

The problem is that uninstalls might happen by simply removing the files without using any package manager or uninstall tool.

This issue seems like a perfect match for the command object design pattern including undo support.

The basic idea would be that each install script no longer is plain PHP included by a setup class, but instead instantiates some install-command objects that can be logged and be reverted using the undo operation.

A developer creating such a script would need to supply the code for the execute method to run the installation logic, and the undo method for the unsinstall.

From a framework perspective the target would be to support those commands in such a way that the undo even works if the module files have been removed. This might include transformation into core install-command classes or PHP object serialization.

Regarding the issue of data-loss sue to automatic uninstallation, that remains the responsibility of the module creator, who might choose to drop a created table created during the execute operation for the undo, or he might choose not to. The main guideline should be that after uninstallations the system must remain in a functional state. So for example, if any attributes with backend, source or frontend models where added, the undo operation should either remove those attributes or simply clear their attribute model properties.

waynetheisinger commented 10 years ago

Genius... Mage2 Core team? P.S. anyone interested in reading about the command object design pattern, I've saved you a google... :stuck_out_tongue_winking_eye:

waynetheisinger commented 10 years ago

I wonder, can someone from the Mage2 core team at least indicate that they've read this thread (even if its too early to comment on how they will attack the problem.)

As an Enterprise customer this problem is EXTREMELY important to me, an enterprise bit of software that becomes unstable when uninstalling additional modules, doesn't really deserve to be called Enterprise level software...

barbazul commented 10 years ago

In addendum to @Vinai excellent vision of how install/upgrade/downgrade scripts should work, I'd also add that the current trigger of installs on a page hit is not correct.

I think Wordpress does this in a safer, more robust way (there is no other circumstans where you will read something like this from me, I hate WP). Setting the migration process itself aside (which is an awful hack that compares the SHOW CREATE against a string in the installer) WP detects version variations and notifies the admin in the corresponding plugins panel. This panel has a list of all installed plugins with options to install, uninstall, activate, deactivate and upgrade the plugins.

I think admin control in this process is crucial to avoid any errors in the installation to surprise visitors and it would also allow us developers to include error messages and debug information in case something goes wrong as we will expect an admin user to read them on the other end.

waynetheisinger commented 10 years ago

again In addendum to @Vinai suggestion - I've just been told about this http://www.liquibase.org/index.html its a opensource framework for making schema changes to databases in a way that they can be source controlled and contain rollback logic, Supports XML, YAML, JSON and SQL formats, this seems a perfect fit to be used by the command object.

Edit * had asked for core team to read but had missed that it had already been tagged as a suggestion *

Vinai commented 10 years ago

Just quick note that even if Magento does not support rollbacks of upgrade scripts, there is an alternative process to not use browser requests to trigger upgrade scripts, but instead do so via command line scripts. This much safer approach was first documented by @colinmollenhour in https://gist.github.com/colinmollenhour/2715268 .

clockworkgeek commented 10 years ago

@Vinai is that a separate request yet? If not please do so. I would love that feature too, especially when used as a post-deployment web hook.

Vinai commented 10 years ago

It already is part of Magento 1, so it already should be part of Magento 2 as well. I only added this as as a related bit of information.

alankent commented 10 years ago

Just commenting on the "is M2 team listening" - "yes".

We are working through the install / uninstall workflow at present, focussing on install first then uninstall. Ultimately need to make sure code executes before a module is removed, however that code is written.

Personally I am still thinking through how much you tie into Composer (e.g. do you try to stop removing a module that still has database schema changes in place?) I suspect not. Two sample use cases are (1) simple WebUI to add and remove modules.to make it easy for basic usage, and (2) command line tools for advanced users.

For (1) the WebUI can ask the user before removing a module and make sure DB changes are removed. (E.g. run 'uninstall' script.) The WebUI will also call Composer to remove code (when we we to it - does not exist today).

For (2) things get more complicated with the more advanced deployments. Composer I see more as for use in a developer's work environment - to assemble the code base for a site. You then ship the code base to your test/staging/prod environments. This phase probably would not use Composer. If you have a cluster of web nodes you also want to make sure the database schema is upgraded once (not per web node). So most likely we just rely on developers using command lines tools in the right order. That is, the developer should remember to use the schema downgrade tool in production before removing the module from the code base.

But we are purposely separating reconfiguring a site (adding/removing modules) from the Merchant's administration tasks (creating products etc) to make things a bit safer, especially in production.

So yes, uninstall is being thought about and these sorts of threads are being watched. We are changing around the install/uninstall process in M2 compared to M1 so things will not be exactly the same. Still working on the install process first however to make sure we can support the two use cases above properly.

sylvainraye commented 10 years ago

I didn't read the whole issue, I'm short in time but we developed during a hackathon such a solution to uninstall Magento Extension for 1.x via the backend.

Here is the source code: https://github.com/magento-hackathon/MageTrashApp.

We use the core resource to uninstall database changes from a uninstall.php script located into sql_setup/myresource/uninstall.php and remove files via a uninstall.txt file, you may get some inspiration or adapt it.

my 50 cts

alankent commented 10 years ago

Thanks! We will certainly have a look. We are reworking the installation process first, then looking at uninstall. So it is topical (or at least will be in the not too distant future.)

djmattyg007 commented 9 years ago

Aren't SQL and data rollback scripts already supported in Magento 1? Has this functionality been removed from Magento 2?

clockworkgeek commented 9 years ago

@djmattyg007 No, but there was a hackathon attempt to introduce it. See MageTrashApp

DavidBruchmann commented 9 years ago

For the case that rollback should work even when an extension is deleted manually there is a copy required or at least the archive of that plugin / extension. This seems resembling to the kind how windows is doing it with updates: everything is stored in a data-store and installation is done extra. Might be a comfortable feature for some, but it's blowing up the data-amount. For backups it shouldn't be required perhaps to save every plugin twice if everything is running and ok, so a backup-script would be advisable too.

On Fri, Dec 12, 2014 at 7:03 PM, djmattyg007 notifications@github.com wrote:

Aren't SQL and data rollback scripts already supported in Magento 1? Has this functionality been removed from Magento 2?

— Reply to this email directly or view it on GitHub https://github.com/magento/magento2/issues/395#issuecomment-66764599.

colinmollenhour commented 9 years ago

I am generally against automatic rollback. Manual rollback I am open to, but I wouldn't really care for it either. Far too many drawbacks for far too little benefit.

In a production environment I wouldn't trust uninstall scripts to not break my database. Uninstalling an extension should be done just as carefully as installing an extension which means testing first in another environment and knowing ahead of time what steps need to be taken (e.g. removing references to removed models in eav attribute "backend_model" column, re-indexing certain indexes, etc.). Between versions it is also highly likely that many changes can't be reversed so people might have the expectation that they can upgrade and if it breaks just roll back to the previous versiona nd all will be fine when in reality they could dig themselves a deeper hole.

In a development environment I'd rather just reload a snapshot if I decide to not take an extension to production than rely on an uninstall script which is likely to be half-baked and poorly maintained. Similarly if I am switching branches that have incompatibilities then I'd rather use snapshots to fix the issue.

There are other concerns too, like what if I am just removing the extension temporarily and don't want to lose all of the data related to it? There is just too much potential for destruction just for the minor additional convenience of being able to uninstall extensions. Installing extensions should generally be viewed as a one-way street, not something you do frequently IMO.

I think many of the issues people have would be resolved if Magento were more resilient to errors caused by the sudden removal of extensions, though. For example, if a product attribute depends on a given model that cannot be resolved then an error should be logged rather than an uncaught exception or fatal error.

kalenjordan commented 9 years ago

I think many of the issues people have would be resolved if Magento were more resilient to errors caused by the sudden removal of extensions, though. For example, if a product attribute depends on a given model that cannot be resolved then an error should be logged rather than an uncaught exception or fatal error.

Good point. We need to get that logged as a github issue if it's not already.

djmattyg007 commented 9 years ago

@clockworkgeek then what does this code do? https://github.com/OpenMage/magento-mirror/blob/magento-1.9/app/code/core/Mage/Core/Model/Resource/Setup.php#L450

clockworkgeek commented 9 years ago

Not a lot. It looks like an unfinished feature. If you follow _modifyResourceDb you will see that the inevitable outcome is to return false.

Also it's unclear what would trigger _rollbackResourceDb. I think only uninstalling the extension and replacing with an older version - with the relevant version number in config.xml - could do it. The Connect Manager has no downgrade option and it doesn't call any setup code before removing files so there is no chance for an uninstall script to execute.

buskamuza commented 9 years ago

When a you deliver a new version of your modules, you need to be able to rollback in case something goes not as expected

I'd not recommend to rely on uninstall/rollback in this case because, if something went not as expected, it means that the DB may be in an unexpected state, so uninstall/rollback may work in unexpected way too.

it's easy with Doctrine Migrations.

Doctrine Migrations can be used for schema only. The bigger problem is to rollback data. And still, I'm not sure that it's so easy to migrate schema - as I remember, Doctrine doesn't distinguish "removing of a column + adding new column" and "renaming a column", so most likely, if you want to rename a column, all the data in it will be lost.

Question to @all: how do you imagine workflow of uninstallation a module(s)?

  1. Data Uninstall Tool. Uninstall tool that uninstalls DB schema, DB data, media + manual removal of the code. It's up to developer how to remove the code (composer remove, rm -rf module, custom script)
  2. All-in-one Uninstall Tool. Uninstall tool that uninstalls DB schema, DB data, media, removes the code, modifies composer.json (if necessary), etc.
  3. Your option
Vinai commented 9 years ago

My preference would be your option 1, the Data Uninstall Tool.

I think it would work great if Magento would provide uninstall hooks to modules. These hooks would be triggered by the uninstall tool and leave the precise actions that are applied to the modules developer.

Once the triggerd actions complete, the module should be left in a deactivated state, so the files can be removed later.

Re-enabling the module at this stage would be like a fresh install.

waynetheisinger commented 9 years ago

Yep exactly what @Vinai said...

waynetheisinger commented 9 years ago

Though in appendum

choukalos commented 9 years ago

Assigned to Tanya; Note that we have an epic slated for 2.0 covering the uninstall of modules ( #MAGETWO-30245 )

ilol commented 9 years ago

@clockworkgeek Planned for Merchant Beta release (summer 2015)

clockworkgeek commented 9 years ago

Thank you @ilol. I look forward to it.