This patch makes the first steps in removing all the upgrade functions that migrate from D6 to D7. It does not remove utility functions that may still be needed in Drupal 8 and keeps in place basic documentation around areas that are guaranteed to be necessary (such as a system_update_8000() and update_fix_d8_requirements()). Further revisions would be great, but this is definitely step 2 for opening up development on Drupal 8 (right after #1089320: Update version strings and constants to 8.x).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quicksketch’s picture

Updated to remove additional PDO checked which aren't necessary (since you already had to have PDO in order to run D7). Not sure if this patch format is correct, git patches aren't quite straight-forward.

wizonesolutions’s picture

Patch applied OK...I'm guessing that to test this, one should attempt to upgrade a D6 database to D8 and see if the script complains? Didn't have a chance to do that.

quicksketch’s picture

Really you just need to upgrade from Drupal 7 to Drupal 8, which right now should only be one update (system_update_8000). You can't ever upgrade between more than one major release at a time.

wizonesolutions’s picture

Oh, OK. I'll try to take a stab at that soon if no one beats me to it.

aspilicious’s picture

Status: Needs review » Needs work

I can't apply this because the format of the patch is a little weird (update.inc gets patched 3 times for example). And as I never updated I followed the update instructions, shouldn't we change garland to stark?

I know that is nitpicking but if we are going to apply this patch I would like it to be done :).

Dries’s picture

quicksketch’s picture

Title: Remove all 7xxx update functions (D6 to D7 upgrade path) » Remove all 7xxx update functions and tests (D6 to D7 upgrade path)
Status: Needs work » Needs review
FileSize
239.23 KB

Well after sitting through an incredibly tedious round of simpletests (literally 10 hours on my local), I found that the upgrade path is totally borked when attempting to update, since all of our tests assume a Drupal 6 database to start, not a D7 one. So I've updated this patch to include removal of the entire "upgrade" directory in the simpletest module. Obviously we'll want to add these tests again as necessary, but right now there is nothing to actually test, since there aren't any differences between D7 and D8 in node, comment, taxonomy or other modules that we were previously testing. The major increase in size is the removal of files that create default Drupal 6 databases. Finally figured out rolling patches against Git origin, so this patch should be good to test and in the right format.

quicksketch’s picture

Arg, sorry that was the same patch as before. Here's the updated one.

quicksketch’s picture

LOL, wow that is intense. I strongly suggest using a visual diffing tool to see that 800KB of that patch is just removing the "modules/simpletest/upgrade" directory.

donSchoe’s picture

subsub

aspilicious’s picture

I never upgraded in my life :) so here is my first attempt. I applied your patch, went fine. There was one file that didn't apply clean.
I followed all the steps, applied new D8 files. I went to my site and it still worked ;). But when I run update.php I get this error.

"The website encountered an error while retrieving http://localhost/drupal8/update.php. It may be down for maintenance or configured incorrectly."

I alrdy changed $update_free_access from FALSE to TRUE. I leave this needs review as I can't compare with a succeeded upgrade.

One remark: if we are keeping some of the database files in simpletest, can we rename them to d7 in stead of d6?
Tnx!

quicksketch’s picture

One remark: if we are keeping some of the database files in simpletest, can we rename them to d7 in stead of d6?

The patch in #8 should remove all the d6 initial database files. Those files are intended to recreate a full Drupal 6 site that can then be tested for upgrading to Drupal 7. Eventually we'll want not just to rename them but actually create a full Drupal 7 database file that provides similar functionality to test the upgrade path to Drupal 8.

Damien Tournoud’s picture

Status: Needs review » Needs work
-/**
- * Implements hook_schema().
- */
-function comment_schema() {

Ew?

Damien Tournoud’s picture

Title: Remove all 7xxx update functions and tests (D6 to D7 upgrade path) » [HEAD BROKEN} Remove all 7xxx update functions and tests (D6 to D7 upgrade path)
Priority: Normal » Critical

Head is currently broken because of this.

Damien Tournoud’s picture

Title: [HEAD BROKEN} Remove all 7xxx update functions and tests (D6 to D7 upgrade path) » [HEAD BROKEN] Remove all 7xxx update functions and tests (D6 to D7 upgrade path)
catch’s picture

Title: [HEAD BROKEN] Remove all 7xxx update functions and tests (D6 to D7 upgrade path) » Remove all 7xxx update functions and tests (D6 to D7 upgrade path)
Priority: Critical » Normal

I love these traditional new release issues.

catch’s picture

Title: Remove all 7xxx update functions and tests (D6 to D7 upgrade path) » [HEAD BROKEN] Remove all 7xxx update functions and tests (D6 to D7 upgrade path)
Priority: Normal » Critical
claar’s picture

I can't get this patch to apply to 8.x with patch or git apply -- I'd simply delete the tests/upgrade directory and submit a new patch, but quicksketch's patch includes changes to a bunch of other files that reference these tests.

QuickSketch, can you address #14 and re-roll this against 8.x head?

claar’s picture

Grr.. I meant address #13.

rfay’s picture

subscribe

mfer’s picture

sub

boombatower’s picture

Status: Needs work » Needs review
FileSize
1.08 MB

Fixed the accidental removal of comment module scheme per #13.

Damien Tournoud’s picture

Status: Needs review » Needs work
 delete mode 100644 modules/simpletest/tests/upgrade/upgrade.test

Let's keep the 'abstract' part of that, please.

-function update_fix_d7_install_profile() {
+function update_fix_d8_install_profile() {

Pretty sure we don't want to keep this.

-/**
- * Implements hook_schema().
- */
-function locale_schema() {

This too :)

The rest looks ok.

boombatower’s picture

Status: Needs work » Needs review
FileSize
1.07 MB

Alright, I cut out the basic test case. I created a separate issue to create a Drupal 7 database for use in testing upgrade path #1109906: Create a Drupal 7 database dump for upgrades (use Drupal 6 dump in 7 as template).

Removed update_fix_d8_install_profile() call and definition.

Reinstated local_schema().

aspilicious’s picture

I'm trying to help but I don't know how to rly test this. I *tried* updating (don't rly know how that works yet, just followed instructions) but I get some server errors while doing that. And when I enable the testing module and I go to the test suite I get the followng strict warning:

Strict warning: Declaration of FieldUIManageFieldsTestCase::setUp() should be compatible with that of FieldUITestCase::setUp() in _registry_check_code() (line 2683 of C:\xampp\htdocs\drupal8\includes\bootstrap.inc).

AND

one file didn't apply clean.

claar’s picture

Status: Needs review » Needs work
-/**
- * Implements hook_uninstall().
- */
-function locale_uninstall() {

Should this function really be removed?

+++ b/includes/update.inc
@@ -68,683 +68,51 @@ function update_check_incompatibility($name, $type = 'module') {
-  include_once DRUPAL_ROOT . '/includes/install.inc';

install.inc must be included or update.php gives a WSOD since drupal_get_installed_schema_version() is undefined.

+++ b/includes/update.inc
@@ -68,683 +68,51 @@ function update_check_incompatibility($name, $type = 'module') {
-  // Allow the database system to work even if the registry has not been
-  // created yet.
-  drupal_bootstrap(DRUPAL_BOOTSTRAP_DATABASE);

Also, drupal_get_installed_schema_version() calls db_query(), so the database must be bootstrapped.

Powered by Dreditor.

quicksketch’s picture

Great feedback guys.

one file didn't apply clean.

Fixed in this reroll.

Should this function really be removed?

No it shouldn't. Restored in this version.

install.inc must be included or update.php gives a WSOD since drupal_get_installed_schema_version() is undefined.

In this case, that entire function (update_prepare_d7_bootstrap) is removed, so we don't need the include statement either, since the entire function is gone. No changes necessary here.

Also, drupal_get_installed_schema_version() calls db_query(), so the database must be bootstrapped.

Good call. Also fixed.

quicksketch’s picture

Status: Needs work » Needs review
claar’s picture

Status: Needs review » Needs work

When running update.php, I still get

Fatal error: Call to undefined function drupal_get_installed_schema_version() in /home/claar/d8-dev/includes/update.inc on line 90 
In this case, that entire function (update_prepare_d7_bootstrap) is removed, so we don't need the include statement either, since the entire function is gone. No changes necessary here.

It's not update_prepare_d7_bootstrap() I was talking about. drupal_get_installed_schema_version() is still undefined on (new) line 90 of update.inc since install.inc is no longer included.

claar’s picture

-function image_requirements($phase) {

I assume we should keep this function? Thanks for the quick patches!

catch’s picture

Category: task » bug
claar’s picture

Status: Needs work » Needs review
FileSize
1.06 MB

A notice was being thrown about $configurable_timezones being undefined due to

-// Temporarily disable configurable timezones so the upgrade process uses the
-// site-wide timezone. This prevents a PHP notice during session initlization
-// and before offsets have been converted in user_update_7002().
-$configurable_timezones = variable_get('configurable_timezones', 1);
 . . . 
$conf['configurable_timezones'] = $configurable_timezones;

The attached patch removes that last usage of $configurable_timezones as well as addressing #29 and #30. Using the attached patch, I was able to successfully update from D7 to D8 without errors or notices.

aspilicious’s picture

- Google chrome still gives me an internal server error (500)
- forefox 4.0 still gives a WSOD

- your patch removes less database test files. Correct?

claar’s picture

@aspilicious: Are you applying the patch to a clean version of the latest 8.x head? If so, please comment out line 339 in the patched update.php, which should be:

ini_set('display_errors', FALSE);

This should turn the WSOD into a useful error message. Thanks!

claar’s picture

Has anyone tried running the tests with these patches since quicksketch in #7? I've never ran tests manually before, but may give it a try if I have time today. (For those who don't know, the test bot won't test patches for 8.x until 8.x itself passes tests, which is part of why this issue is so critical -- it's holding up all 8.x patches from testing).

quicksketch’s picture

I'll run the tests again now. Unfortunately last time I ran the whole suite it took about 8 hours; sit tight.

aspilicious’s picture

Status: Needs review » Needs work

Fatal error: Call to undefined function update_prepare_d7_bootstrap() in C:\xampp\htdocs\drupal8\update.php on line 349

To fix this:
----------
update_prepare_d7_bootstrap(); ==> update_prepare_d8_bootstrap();
update_fix_d7_requirements(); ==> update_fix_d8_requirements();

1) A lot of comments are still using D7
2) files like system.install contains a lot of update functions
3) There where no pending updates in my case... and there should be one (see #3)

aspilicious’s picture

Status: Needs work » Needs review

Ok, update succeeded, eclipse doesn't like to apply 1mb patches.

quicksketch’s picture

Ok, update succeeded, eclipse doesn't like to apply 1mb patches.

Yep, agreed. My patch didn't apply successfully at first either (I also use Eclipse), but so far #32 is looking great after applying successfully. Unfortunately my tests stopped at about 60% when I accidentally restarted Apache. :(

Tests are looking great so far (100% of the ones I got through), but now I have to start it all over again. Another 8 hours... sigh.

boombatower’s picture

Status: Needs review » Reviewed & tested by the community

Alright, well we can commit and if we need to followup then we can do so, but no sense holding this up unless we have some issue with the current patch?

aspilicious’s picture

Status: Reviewed & tested by the community » Needs review

Claar told me he had some failing tests lets wait a few hours!

claar’s picture

Status: Needs review » Reviewed & tested by the community

Actually, the failed test I had was some sort of semaphore lock -- when I re-tested, it passed, so I actually haven't had a test fail yet. I'd love to see this committed as is -- #32 is definitely better than where we're at now, and it would let testbot take a crack at it.

I'm currently re-running the test suite too, but I'm not very good at it -- my last run failed with an out-of-memory error, so I just restarted it doing half the tests at a time.

quicksketch’s picture

Actually, the failed test I had was some sort of semaphore lock -- when I re-tested, it passed, so I actually haven't had a test fail yet. I'd love to see this committed as is -- #32 is definitely better than where we're at now, and it would let testbot take a crack at it.

I had this exact same test fail the first time I ran it too. The drupal_render() test seems to occasionally fail also, but never twice in a row. Seems like some of our tests are a bit fragile. This patch doesn't touch on either drupal_render() or the lock/semaphore system.

So with all tests passing, update.php working as expected, the ability to "upgrade" from Drupal 7 to Drupal 8 all in place, I also agree this is ready to go. It'll be nice to start work on Drupal 8. :)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed this patch to 8.x.

The patch removes some file so I hope I got the git-commands right. I double-checked and it looks like I did.

Now let's follow-up on this patch as necessary. Setting it to 'fixed' for now.

rfay’s picture

Status: Fixed » Needs work

Still one fail on the upgrade tests: http://qa.drupal.org/pifr/test/16628

quicksketch’s picture

Sweet, thanks Dries and rfay! I'll take a look at the remaining tests. Glad to see both the drupal_render() and locking tests passing by testbot though. Strange that they don't pass all the time locally for me.

quicksketch’s picture

Status: Needs work » Needs review
FileSize
384 bytes

Ha! So this bug is caused by the aaa_update_test.tar.gz that we use for testing still containing a "version = 7.x" line in the .info file. :)

This archive file needs to be updated. Place this file at modules/update/tests/aaa_update_test.tar.gz and the tests should pass.

rfay’s picture

Isn't there something wrong with a test that depends on drupal.org?

I'm pinging in #drupal-infrastructure to get this. After it's replaced I'll rerun the test.

webchick’s picture

Title: [HEAD BROKEN] Remove all 7xxx update functions and tests (D6 to D7 upgrade path) » [8.x BROKEN] Remove all 7xxx update functions and tests (D6 to D7 upgrade path)

It's not called HEAD anymore. ;)

rfay’s picture

Title: [8.x BROKEN] Remove all 7xxx update functions and tests (D6 to D7 upgrade path) » [HEAD BROKEN] Remove all 7xxx update functions and tests (D6 to D7 upgrade path)
FileSize
1.54 KB

OK, so I completely misunderstood.

Here's the patch you need to apply to make quicksketch's change, Dries.

DamZ schooled me in this:

git format-patch --binary --full-index --stdout >/path/to/binary_patch.patch

Dries, you can apply in the normal way with

git am patch

or

git apply patch

rfay’s picture

Title: [HEAD BROKEN] Remove all 7xxx update functions and tests (D6 to D7 upgrade path) » [8.x BROKEN] Remove all 7xxx update functions and tests (D6 to D7 upgrade path)

x-post.

I was wondering what we were going to do to change our terminology. SHE has spoken :-)

boombatower’s picture

Status: Needs review » Reviewed & tested by the community

UpdateTestUploadCase passes locally with patch.

rfay’s picture

Ooops, bad commit message in case Dries uses git am. Here's one with a decent commit message. Exactly the same otherwise.

catch’s picture

Bump. 8.x is still broken.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks for the instructions too. :)

webchick’s picture

Title: [8.x BROKEN] Remove all 7xxx update functions and tests (D6 to D7 upgrade path) » Remove all 7xxx update functions and tests (D6 to D7 upgrade path)

Yay!

rfay’s picture

Such a nice green! http://qa.drupal.org/pifr/test/16628

Thanks all for doing this work. Thanks for the commit, Dries.

catch’s picture

Status: Fixed » Active

This patch renamed _update_7000_node_get_types() (and presumably others, did not check) to _update_8000_node_get_types() - those functions are linked to schema version, not Drupal version, so need to be renamed back.

Critical again because this now blocks #1017672: D6 to D7 update process permanently deletes comment bodies and other data, and throws fatal SQL errors.

bfroehle’s picture

Status: Active » Needs review
FileSize
4.13 KB
quicksketch’s picture

@catch: I left those functions in the .install files in case we needed them during the Drupal 8 cycle, but none of these functions are actually called anywhere in Drupal 8. Couldn't we just remove them entirely? I'm not understanding why we need to rename them back, or really why they would have an effect on anything at all if they're not used.

catch’s picture

@quicksketch: I don't like the fact that we have those functions at all, that prompted #1052692: New import API for major version upgrades (was migrate module) (which of course is a very long way off, if it ever happens). I'm not averse to removing them to 8.x but I don't see a good reason to either especially if we have to add them back again.

The functions should be tied to the schema version when they were added, when there's a new schema version for the module, we'd need to add a new function with the new schema name - this allows foo_update_8000() to get a list of node types with one schema, and foo_update_8032() to get a list of node types with a different schema. Question is whether we'll need these specific functions during the 7.x-8.x upgrade path. Damien and chx added these, so maybe they have some feedback.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Yeah catch is totally right please rename them back ASAP.

catch’s picture

Either that or we should revert back completely and postpone this on a working 6.x-7.x upgrade path and #1119094: Add a test to verify the schema matches after a database update.

quicksketch’s picture

/me shrugs. While I think it's silly to have 7000_* named functions lingering around, it's not going to hurt anything renaming them back, since (like I said) they're not actually used anywhere. Seems like if we're going to keep them they should be given appropriate names though. Can someone explain why a function that isn't even used in D8 this is blocking D7 bug fixes?

catch’s picture

They are named on schema version, if the schema relating to those functions doesn't change via the D8 lifecycle, then 7000 will be the appropriate name. If it changes during the D7 cycle, we may need to add 7012 versions of those functions to D7, in which case it will be very silly for the 7000 version to be in D8 but named 8000 - are you going to rename 7012 to 8012 then?

We could remove the functions, but then we'd need to add them back as soon as (for example) someone wants to write an update that adds a field. Either way is a pain in the arse.

catch’s picture

Or to put it a different way:

1. Install Drupal 8, run SELECT name, shema_version FROM system; the schema version for installed modules will be 0, not 8000.

2. Install Drupal 7, 'upgrade' it to Drupal 8 - the schema version will be 0 or 7* for all installed modules.

Therefore there is currently no code in Drupal core at all, that corresponds to a schema version of 8000 yet (edit apparently there is but it is a no-op, however it only applies to system module). And even that update might just set a variable or update a field definition, rather than change the schema (in which case we don't bother changing the functions at all - until we have to).

On why this is blocking D7 bug fixes

- We branched while there were still critical data loss issues in the upgrade path in Drupal 7, then proceeded to diverge the code paths for the upgrade path, while we have a strict policy of fix in HEAD and backport. I don't think we should have branched while this was the case.

- Removing the functions entirely would have not blocked the other issue (it would mean that patch only goes into D7), leaving the functions named correctly would also have not blocked it (the patch would apply to D8 with no dependencies). It is the renaming here that means we are blocking it, because the only way I can make that patch apply is ranting in this issue until it is fixed, or re-rolling that patch to be wrong.

Also this didn't just block D7 patches, committing straight from needs review on http://drupal.org/node/1089320#comment-4237202 broke D8 HEAD too for several days. This doesn't make me feel very optimistic about all the good intentions we had at DrupalCon about a less critically broken development release during the course of the cycle.

quicksketch’s picture

ranting in this issue until it is fixed, or re-rolling that patch to be wrong.

It sounds like this is the philosophical hold-up. The patch could easily be re-rolled with the 8000_* functions and get that committed despite it being "wrong" because of the function name number not matching a schema version.

Well in any case, it's not going to make any difference in D8. Let's just rename the functions back and work under the assumption that they're going to get renamed anyway as soon as we need them (and then we can match the schema version appropriately).

EDIT: Redacted some meanness. Sorry not sure what's gotten into me.

quicksketch’s picture

So catch and I had an IRC chat in which he explained the whole situation to me on why these shouldn't be 8000_* and why we should keep them around. The explanation makes sense and I agree we should rename them back to 7000. Let's just rename them back and be done with it. #59 is still ready to go.

catch’s picture

I opened an issue for better docs at #1134088: Properly document update-api functions, this patch needs to go in before work can start on that, since it's the same lines of code, however I'll make a start on it once committed.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

catch’s picture

Status: Fixed » Reviewed & tested by the community

Patch was not committed.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Odd. Committed it once again! ;-)

See http://drupalcode.org/project/drupal.git/commit/f5e8a63.

yched’s picture

Ah - that's because it initially got committed along with #1111288: wrong instance definition in field_ui.test, and got rolled back over there :-p. My bad.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.