Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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).
Comment | File | Size | Author |
---|---|---|---|
#59 | 1097100-fixup-by-bfroehle-Rename-helper-functi.patch | 4.13 KB | bfroehle |
#53 | drupal.new_tarball_1097100_53.patch | 1.6 KB | rfay |
#50 | drupal.new_tarball_1097100.patch | 1.54 KB | rfay |
#47 | aaa_update_test.tar_.gz | 384 bytes | quicksketch |
#32 | 1097100-remove-updates-32.patch | 1.06 MB | claar |
Comments
Comment #1
quicksketchUpdated 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.
Comment #2
wizonesolutionsPatch 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.
Comment #3
quicksketchReally 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.
Comment #4
wizonesolutionsOh, OK. I'll try to take a stab at that soon if no one beats me to it.
Comment #5
aspilicious CreditAttribution: aspilicious commentedI 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 :).
Comment #6
Dries CreditAttribution: Dries commentedCommitted #1089320: Update version strings and constants to 8.x.
Comment #7
quicksketchWell 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.
Comment #8
quicksketchArg, sorry that was the same patch as before. Here's the updated one.
Comment #9
quicksketchLOL, 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.
Comment #10
donSchoe CreditAttribution: donSchoe commentedsubsub
Comment #11
aspilicious CreditAttribution: aspilicious commentedI 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!
Comment #12
quicksketchThe 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.
Comment #13
Damien Tournoud CreditAttribution: Damien Tournoud commentedEw?
Comment #14
Damien Tournoud CreditAttribution: Damien Tournoud commentedHead is currently broken because of this.
Comment #15
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #16
catchI love these traditional new release issues.
Comment #17
catchComment #18
claar CreditAttribution: claar commentedI 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?
Comment #19
claar CreditAttribution: claar commentedGrr.. I meant address #13.
Comment #20
rfaysubscribe
Comment #21
mfer CreditAttribution: mfer commentedsub
Comment #22
boombatower CreditAttribution: boombatower commentedFixed the accidental removal of comment module scheme per #13.
Comment #23
Damien Tournoud CreditAttribution: Damien Tournoud commentedLet's keep the 'abstract' part of that, please.
Pretty sure we don't want to keep this.
This too :)
The rest looks ok.
Comment #24
boombatower CreditAttribution: boombatower commentedAlright, 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().
Comment #25
aspilicious CreditAttribution: aspilicious commentedI'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.
Comment #26
claar CreditAttribution: claar commentedShould this function really be removed?
install.inc must be included or update.php gives a WSOD since drupal_get_installed_schema_version() is undefined.
Also, drupal_get_installed_schema_version() calls db_query(), so the database must be bootstrapped.
Powered by Dreditor.
Comment #27
quicksketchGreat feedback guys.
Fixed in this reroll.
No it shouldn't. Restored in this version.
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.
Good call. Also fixed.
Comment #28
quicksketchComment #29
claar CreditAttribution: claar commentedWhen running update.php, I still get
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.
Comment #30
claar CreditAttribution: claar commentedI assume we should keep this function? Thanks for the quick patches!
Comment #31
catchComment #32
claar CreditAttribution: claar commentedA notice was being thrown about $configurable_timezones being undefined due to
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.
Comment #33
aspilicious CreditAttribution: aspilicious commented- 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?
Comment #34
claar CreditAttribution: claar commented@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:
This should turn the WSOD into a useful error message. Thanks!
Comment #35
claar CreditAttribution: claar commentedHas 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).
Comment #36
quicksketchI'll run the tests again now. Unfortunately last time I ran the whole suite it took about 8 hours; sit tight.
Comment #37
aspilicious CreditAttribution: aspilicious commentedFatal 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)
Comment #38
aspilicious CreditAttribution: aspilicious commentedOk, update succeeded, eclipse doesn't like to apply 1mb patches.
Comment #39
quicksketchYep, 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.
Comment #40
boombatower CreditAttribution: boombatower commentedAlright, 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?
Comment #41
aspilicious CreditAttribution: aspilicious commentedClaar told me he had some failing tests lets wait a few hours!
Comment #42
claar CreditAttribution: claar commentedActually, 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.
Comment #43
quicksketchI 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. :)
Comment #44
Dries CreditAttribution: Dries commentedCommitted 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.
Comment #45
rfayStill one fail on the upgrade tests: http://qa.drupal.org/pifr/test/16628
Comment #46
quicksketchSweet, 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.
Comment #47
quicksketchHa! 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.
Comment #48
rfayIsn'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.
Comment #49
webchickIt's not called HEAD anymore. ;)
Comment #50
rfayOK, 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
Comment #51
rfayx-post.
I was wondering what we were going to do to change our terminology. SHE has spoken :-)
Comment #52
boombatower CreditAttribution: boombatower commentedUpdateTestUploadCase passes locally with patch.
Comment #53
rfayOoops, bad commit message in case Dries uses git am. Here's one with a decent commit message. Exactly the same otherwise.
Comment #54
catchBump. 8.x is still broken.
Comment #55
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks for the instructions too. :)
Comment #56
webchickYay!
Comment #57
rfaySuch a nice green! http://qa.drupal.org/pifr/test/16628
Thanks all for doing this work. Thanks for the commit, Dries.
Comment #58
catchThis 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.
Comment #59
bfroehle CreditAttribution: bfroehle commentedComment #60
quicksketch@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.
Comment #61
catch@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.
Comment #62
chx CreditAttribution: chx commentedYeah catch is totally right please rename them back ASAP.
Comment #63
catchEither 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.
Comment #64
quicksketch/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?
Comment #65
catchThey 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.
Comment #66
catchOr 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.
Comment #67
quicksketchIt 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.
Comment #68
quicksketchSo 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.
Comment #69
catchI 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.
Comment #70
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.
Comment #71
catchPatch was not committed.
Comment #72
Dries CreditAttribution: Dries commentedOdd. Committed it once again! ;-)
See http://drupalcode.org/project/drupal.git/commit/f5e8a63.
Comment #73
yched CreditAttribution: yched commentedAh - 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.