Migration 'down' method does not successfully execute after removing extensions

[Ralkage]

Bug Report

Current Behavior I wasn't sure how to phrase the title properly but I'll try to explain the issue at hand. Currently, I've been seeing a lot of SQL foreign key constraints errors being reported on Discuss.

Upon trying to reproduce an issue that @mcutting was having with FoF/Gamification, I noticed that when I ran composer remove fof/gamification the associating tables that were created at the beginning of the install never dropped as indicated by the down method of the related migration for that table. My next test was to try this on a bundled extension such as Flarum/Tags and the same thing happened, the tags table never dropped even though it indicates that it should as per [these lines]().

Because of the fact that the "down" method doesn't seem to successfully execute when uninstalling an extension, this causes issues later when the extension is reinstalled and foreign key constraints are added back once more. The SQL constraint error occurs because it is trying to associate the related records that existed in the table that never dropped and the columns the constraint references in another table (such as posts and discussions) have records that no longer exist.

In turn, if post_id in the post_votes table of Gamification can't find a matchingid in the posts table because that post was deleted, it will throw that SQL constraint error.

Steps to Reproduce

1. Run composer remove flarum/tags in the root directory of your flarum installation.
2. Run USE <flrum_db_name>; using whatever method you use to run SQL queries on your databases.
3. Run SHOW TABLES; using that same database to see if the tags table is still present after uninstalling the Tags extension.
   • Alternatively, you can run the folowing query:

     SELECT * 
     FROM information_schema.tables
     WHERE table_schema = '<flrum_db_name>' 
     AND table_name = 'tags'
     LIMIT 1;

If it does not return any results, then the tags table had successfully been dropped which is what we want to have happened.

Expected Behavior When an extension is uninstalled completely, the tables that were created by the extension through migration files should be dropped as indicated by their "down" methods.

Screenshots N/A

Environment

• Flarum version: 0.1.0-beta.10
• Website URL: Locally hosted installation
• Webserver: Apache
• Hosting environment: Localhost
• PHP version: 7.3.1
• Browser: Firefox 69.0.1 (64-bit)

Flarum core 0.1.0-beta.10
PHP version: 7.2.14 (should be 7.3.1)
Loaded extensions: Core, bcmath, calendar, ctype, date, filter, hash, iconv, json, SPL, pcre, readline, Reflection, session, standard, mysqlnd
, tokenizer, zip, zlib, libxml, dom, PDO, bz2, SimpleXML, xml, wddx, xmlreader, xmlwriter, openssl, curl, fileinfo, gd, gettext, gmp, intl, im
ap, ldap, mbstring, exif, mysqli, Phar, pdo_mysql, pdo_sqlite, soap, sockets, xmlrpc, xsl
+----------------------+----------------+------------------------------------------+
| Flarum Extensions    |                |                                          |
+----------------------+----------------+------------------------------------------+
| ID                   | Version        | Commit                                   |
+----------------------+----------------+------------------------------------------+
| flarum-approval      | v0.1.0-beta.8  |                                          |
| flarum-bbcode        | v0.1.0-beta.8  |                                          |
| flarum-emoji         | v0.1.0-beta.10 |                                          |
| flarum-lang-english  | v0.1.0-beta.10 |                                          |
| flarum-flags         | v0.1.0-beta.9  |                                          |
| flarum-likes         | v0.1.0-beta.9  |                                          |
| flarum-lock          | v0.1.0-beta.9  |                                          |
| flarum-markdown      | v0.1.0-beta.10 |                                          |
| flarum-mentions      | v0.1.0-beta.10 |                                          |
| flarum-statistics    | v0.1.0-beta.9  |                                          |
| flarum-sticky        | v0.1.0-beta.9  |                                          |
| flarum-subscriptions | v0.1.0-beta.9  |                                          |
| flarum-suspend       | v0.1.0-beta.9  |                                          |
| fof-strikes          | dev-master     | dcc4a978b2a401ebb687143f1d08f36e9fab2f78 |
| fof-subscribed       | dev-master     | cc086c8838ce5d15c302ba3f174cf3c02cb7cfbe |
| flarum-auth-github   | v0.1.0-beta.9  |                                          |
+----------------------+----------------+------------------------------------------+
Base URL: http://b10.local
Installation path: C:\wamp64\www\b10.local
Debug mode: off

Possible Solution

For extension developers, if you're adding foreign key constraints of any kind, it should be advised that logic be added to remove these records when your extension is uninstalled and reinstalled again. The Tags extension uses a logic that either removes these records that have no related foreign key id's or simply sets the records in that column as NULL as indicated here.

As per the main issue with the "down" method, I believe the method fails because the foreign key constraint(s) never get deleted due to the nature of how migrations run (by date/time). Before the table drops, the foreign key constraints should either be removed or the values of these columns should be set to NULL (or even empty) so that the foreign key constraints can be deleted when the "down" method for that added constraint through it's migration file runs its course.

Additional Context N/A

[Ralkage]

Wow, I feel very stupid about this and also wasting my time writing this up.

@datitisev explained to me on the Discuss Discord server that the down method only runs when the Uninstall button is clicked on after an extension is disabled from the Admin CP and not when you run composer remove vendor/package-name. I've been living in the extension development life for way too long and forgot how to be an end-user 🙄 😆

Not sure if y'all want to go ahead and close this but it can be quite confusing since we've always been told to remove extensions using composer (after disabling them in the Admin CP) and rarely uninstall them through the Admin CP since we typically install them using composer in the same manner.

The foreign key constraint fix/workaround should probably be documented somewhere nonetheless as this is something extension developers don't typically add to their migrations when adding foreign key constraints later down the line.

Your call ❤️

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum. In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!

[stale[bot]]

We are closing this issue as it seems to have grown stale. If you still encounter this problem with the latest version, feel free to re-open it.

[the-turk]

I'll vote for current behaviour because of this scenario:

I've this Diff extension and I need to be sure if it's still working after a brand new core release. So it conflicts and doesn't allow users to update their core package and users should wait for me to test everything and update version constraints. If they don't want to wait, then they can remove this extension by running composer remove command and solve conflictions temporarily without touching anything on database side. But if migration down method also runs when you run composer remove command, extensions will lose all of their records forever - users should deal with it after every single Flarum release.

[tankerkiller125]

@the-turk we discussed this internally in our team chats, during those discussions an idea what brought forth to ask the user on package removal if they also wanted to remove the database entries. The idea being that the user could say no and the data would remain but the package would be removed (current behavior). But if the user said yes the data would be removed and so would the package.

I believe that this would take care of your concern?

[the-turk]

@the-turk we discussed this internally in our team chats, during those discussions an idea what brought forth to ask the user on package removal if they also wanted to remove the database entries. The idea being that the user could say no and the data would remain but the package would be removed (current behavior). But if the user said yes the data would be removed and so would the package.

I believe that this would take care of your concern?

Perfect solution 👌 Thanks for the info!

[clarkwinkelmann]

Why was this re-opened @franzliedke ?

The point raised by @tankerkiller125 above is not mentioned at all in the original report, is this the reason why we re-opened it?

I suggest we open a new issue about asking-during-composer-remove and close this one.