This came up over at #1777070: Refactor and clean up source string location handling.

The API docs for this function state:

"No access check has been performed when this function is called, so no irreversible changes to the database are made here."

However, we are currently making all manner of irreversible changes, including dropping tables and fields, changing field names without changing them back, etc. (See sub-functions such as http://api.drupal.org/api/drupal/core%21includes%21update.inc/function/u...

Unless our philosophy on this has shifted on this (which I don't personally believe it should, since it should be viable to update your files to D8, attempt to load update.php (or worse, have some anonymous user do it), get an error, and move your files back to D7 without destroying your site), I believe that makes this a critical bug until we resolve this. We could probably argue about it being postponed until after feature freeze, though.

CommentFileSizeAuthor
#11 documentation-1816864-11.patch5.26 KBmahaprasad
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Question is how do we want to make D8 bootstrap without any of those changes? Eg. without the language table renamed it would never be able to go past language bootstrap and never reach any of the update functions. Changes to this file to this effect started in December 2011 with this commit: http://drupalcode.org/project/drupal.git/commitdiff/13830d022c2b43f7cec5... (and have been updated since then with various patches to make language bootstrapping work).

chx’s picture

Can we drop the uid 1 check for major upgrades and then we can access check here manually?

catch’s picture

We should just completely drop the concept of 'upgrading' a live database and require people to upgrade backups, then it doesn't matter if this function makes changes to the db.

webchick’s picture

How would you suggest we do that? Put in an interstitial that says "FFS DO NOT DO THIS"? (Actually I don't think we can do an interstitial without bootstrapping which is the cause of this issue). Update UPGRADE.txt (is that enough)?

catch’s picture

UPDATE.txt might be enough.

Given we were deleting comment bodies for the first six months after 7.0 was released I don't think we can continue to tell people to upgrade their live database anyway.

chx’s picture

Even for minor updates it was always a wise choice to backup first. And yes, I agree.

chx’s picture

Title: update_prepare_d8_bootstrap() makes irreversible changes to D7 databases, so update UPGRADE.txt accordingly » update_prepare_d8_bootstrap() makes irreversible changes to D7 databases
Component: documentation » database update system
Category: task » bug
Priority: Major » Critical

Do you remember our first doc maintainer? "Test site, always start with a test site." - sepeck. guide is now here http://drupal.org/node/839648

webchick’s picture

Title: update_prepare_d8_bootstrap() makes irreversible changes to D7 databases » update_prepare_d8_bootstrap() makes irreversible changes to D7 databases, so update UPGRADE.txt accordingly
Component: database update system » documentation
Category: bug » task
Priority: Critical » Major

Ok, then let's change this to a major task to update UPGRADE.txt instructions to inform people to do this from a backup database instead.

I could tag this as novice if someone would sketch out the basic gist of instructions here. I've always done it from a live DB (after testing on localhost first) so I don't really know how to do it any other way.

Gábor Hojtsy’s picture

I think we can display an interstitial that says "FFS DO NOT DO THIS" IF we hardcode the HTML for that in the update (not require a tpl.php, no localization, etc.). To do access checks, looks like we might need to bootstrap to a level that needs languages to be bootstrappable which requires extensive schema changes.

I think its not very professional to maintain the possibility for live sites to be swapped their code and then risk anonymous users visiting update.php (or index.php for that matter). I think even visiting index.php can do hardly reversible damage to the site, eg. the filter list to be refreshed/regenerated and other bugos (D8 format) data saved into the DB, caches regenerated, etc. So maintaining a feature that update.php can be visited by anonymous without priviledges to run the update and not have any effect still letting people to back out the code update is IMHO futile in light of how the rest of Drupal works.

Gábor Hojtsy’s picture

Title: update_prepare_d8_bootstrap() makes irreversible changes to D7 databases » update_prepare_d8_bootstrap() makes irreversible changes to D7 databases, so update UPGRADE.txt accordingly
Component: database update system » documentation
Category: bug » task
Priority: Critical » Major

Crosspost.

mahaprasad’s picture

Status: Active » Needs review
FileSize
5.26 KB

Created documentation patch. Please review.

Thank You!

jhodgdon’s picture

Status: Needs review » Needs work

There are spaces at the ends of the new lines. Please remove them, thanks!

But... Also the UPGRADE.txt file already says at the top in the Before you Start section:

  * Make a full backup of all files, directories, and your database(s) before
    starting, and save it outside your Drupal installation directory.
    Instructions may be found at http://drupal.org/upgrade/backing-up-the-db

So I don't think we need to make this recommendation as an extra step. I'm not sure what the people discussing this issue above intended, but I don't think this was it... Maybe the people discussing this above could comment on what they think needs to be added to UPGRADE.txt really?

Gábor Hojtsy’s picture

The idea/problem was that "starting" means *clicking the start button* on update.php after reviewing what is going to happen, not *loading update.php* itself. Currently loading update.php itself makes irreversible changes so it can bootstrap Drupal to the level to display the page.

jhodgdon’s picture

Ah, OK.... So, let's think a minute. So someone has a Drupal 7 site. They are going to try to update to Drupal 8 via update.php. They get to the point where they delete all the 7 files and copy in the 8 files, and navigate to update.php, and whammo, their Drupal 7 database is irreversibly changed even though they didn't click the Start button/link yet.

If they had followed the instructions at the top of the file, they would have made a database/file backup before they started, and they'll be OK.

But it sounds like just before step 14 of the Major Upgrade instructions (the one where you actually navigate to update.php), we should put a step that says something like:

"Before visiting the update.php script, double-check that you have made a database backup. Even just visiting update.php for the first time will make changes to your database that are incompatible with Drupal 7."

or something to that effect -- right?

jhodgdon’s picture

Has someone made this note in the on-line drupal.org upgrade instructions as well? And would it apply to Drupal 7 or earlier versions?

tstoeckler’s picture

Yes, this applies to Drupal 7 at least, and I assume Drupal 6 as well.

David_Rothstein’s picture

Yes, this applies to Drupal 7 at least

I'm not sure I see anything irreversible in update_prepare_d7_bootstrap(), actually. You should still be able to switch back to a Drupal 6 codebase after that runs.

Though I don't claim to have looked very carefully...

webchick’s picture

Yeah, if there is anything it's an accident. We made a very concerted effort in D7 and D6 to make sure this function could be run painlessly on sites that might've had to back out of the upgrade. It pains me that we're apparently unable to do this in D8... will only add to the number of huge problems people can have. :\ "oops you should've had a backup" only makes us feel better about ourselves, doesn't actually help anyone.

Gábor Hojtsy’s picture

As said above, update.php can start by fpassthru()-ing (or equivalents) a static HTML splash page saying "BEWARE". That would completely satisfy the problem case *itself* I think. As explained by @jhodgdon I agree that if the user went all the way to replace the codebase with the Drupal 8 codebase, and run code from the Drupal 8 codebase (it is also possible their cron or some other thing hit frontend pages which is just as dangerous to existing data), then it is not very realistic to expect a clean way out.

chx’s picture

I have now reviewed the update functions and I am fairly close to raise this to critical or file a critical to make the changes reversible. Renaming? That's fine. Dropping things? Why? Why do you drop data? Realistically, how much disk space and memory is eaten in the languages table by those columns? Given the number of rows in languages table, a kilobyte? Let's be gracious: ten kilobytes. I thought it's 2013 not 1983. Why on earth do we drop anything? I will need to review more but I thought we had a policy of not dropping anything in updates.

chx’s picture

But then again, we might say that we want to change that policy but I would rather not. Backups or not, partial restores are hard and in general, I would rather not drop anything unless badly necessary (and I do not know why it'd be).

Gábor Hojtsy’s picture

@chx: there are irreversible changes anyway, eg. dropping indexes in url_alias, renaming a column and adding new indexes. While we can move some of the data dropping later, for the language bootstrap to be able to run, the language bootstrap needs the settings at the new place in config, so we would migrate the settings in the update.php bootstrap pepare to the new place but not yet remove the old place. Update code in Drupal usually does the migration of data to new places at the same code where it removes it from the old place. So while that would make it somewhat easier to roll back, there is still a huge number of things to roll back and undo.

The languages table for example is renamed to language, the language column in that table is renamed to langcode and new languages are added at the start of the upgrade. Those are at least needed to bootstrap with the D8 bootstrap. The new languages would need to make up fake data for the columns that would be removed later and do other temporary stuff just to avoid a small part of the update happening, so it is 20% easier to roll back the update, while the 80% is already pretty painful manually.

tstoeckler’s picture

Wow, missed quite a couple of comments. Sorry for posting #16 without checking. Indeed in D7 update.php (on the front page) does not make irreversable changes, it simply creates some tables. I remembered that it makes some changes (because I found that strange seeing it for the first time), and simply assumed the 'irreversable' part. That was completely bogus, sorry.

xjm’s picture

Issue summary: View changes
Status: Needs work » Closed (duplicate)

It's now in scope for #2168011: Remove all 7.x to 8.x update hooks and disallow updates from the previous major version to remove update_prepare_d8_bootstrap() entirely, so marking as a duplicate of that issue.