Problem/Motivation
At DrupalCon Prague, we decided not to provide a Drupal 7 to Drupal 8 upgrade path using the database update system, and to instead provide data migration using a Drupal 8 data migration API based on the Migrate module. However, hundreds of 7.x to 8.x update hooks remain, which is confusing to core contributors and module developers. When discussing the criteria for a beta release, @catch, @Dries, @Moshe Weitzman, and myself agreed that these hooks should be removed from the codebase before beta 1 to reduce confusion.
Proposed resolution
- Disallow upgrades from 7.x to 8.x.
- Remove all implementations of
hook_update_N()
andhook_update_dependencies()
. - Remove unused API functions for the 7.x to 8.x migration path.
The approach has consensus from the Migrate module maintainers.
Remaining tasks
- Latest patch needs code review.
- Check for references to removed upgrade helpers in change records.
- Post g.d.o/core announcement about filing migration issues after reviewing with core maintainers (see #36).
API changes
- Any upgrades from 7.x to 8.x are now disallowed. The user will receive a requirements error if attempting to update with a Drupal 7 database.
- The partial 7.x to 8.x upgrade path using the database update system is no longer provided in Drupal 8.
hook_update_N()
functions now start at_###1
to reserve#000
for the minimum installed schema with no updates. Examples from this patch:- function update_test_1_update_8001()
- function update_test_2_update_8001()
- function update_test_3_update_8001()
- function update_script_test_update_8001()
A requirements error will be displayed when updating if a module defines a
hook_update_8000()
.- Several update helper functions are removed.
- function update_prepare_stored_includes()
- function update_prepare_d8_language()
- function update_prepage_d8_bootstrap()
- function update_fix_d8_requirements()
- function update_variable_get()
- function update_variable_set()
- function update_variable_del()
- function update_variables_to_config()
- function update_install_default_config()
- function update_variables_to_state()
- function _update_8000_entity_get_display()
- function _update_8000_entity_get_form_display()
- function _update_8003_field_create_field()
- function _update_8003_field_create_instance()
- function _update_8006_field_write_data_sql()
- function _update_7000_node_get_types()
Additionally,
update_extra_requirements()
is replaced with specific functionsupdate_system_schema_requirements()
andupdate_settings_file_requirements()
. - A couple constants are altered as well.
- Removed
- const SCHEMA_INSTALLED = 0;
- const REQUIRED_D7_SCHEMA_VERSION = '7069';
- Added
- const \Drupal::CORE_MINIMUM_SCHEMA_VERSION = 8000;
- Removed
Comment | File | Size | Author |
---|---|---|---|
#154 | Untitled.png | 21.06 KB | KyleNakhul |
#132 | update-2168011-119-do-not-test.patch | 89.79 KB | xjm |
#130 | update-2168011-119.patch | 295.79 KB | jessebeach |
#127 | interdiff.txt | 17.61 KB | jessebeach |
#127 | update-2168011-127.patch | 319.19 KB | jessebeach |
Comments
Comment #1
catchThe order doesn't bother me at all - these are only going to be there for a while to save people looking back through git history.
Comment #2
xjmWe might also need to update some change records for the removed upgrade helpers. E.g., I ran into https://drupal.org/node/2012896.
Edit: There's also a bunch more stuff in update.inc that we should discuss what to do with.
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedIf we want to keep supporting minor updates (I assume we do), we need to pin the schema version of all the modules.
We needs two things:
The lowest version in 8xxx is 8000, so we should default to that. But that means that
*_update_8000
needs to be banned, the first real update need to be8001
.So we need three changes in addition to removing updates:
Drupal\Core\Extension\ModuleHandler::install()
, above the "last removed" handling, add a$version = max($version, \Drupal::CORE_COMPATIBILITY * 1000);
;update_get_update_list()
, throw an exception if an update function in 8000 exists;update_get_update_list()
, above the "last removed" handling, add a check for$schema_version < \Drupal::CORE_COMPATIBILITY * 1000
;In addition to all of this, we need to decide what to do with the update tests.
Comment #5
xjmThanks @Damien Tournoud!
The test coverage for the 7.x -> 8.x upgrade path is already removed; is that what you're referring to? I left update functionality self-tests alone because we're still going to use them for 8.0.0 -> 8.0.1 (and possibly 8.0-beta1 -> 8.0-beta2), etc.
Comment #6
xjmNoticed #2168101: UpdateModuleHandler does not follow coding standards for switch statements and is hard to read while working on this.
Comment #7
xjmGot a little overzealous in the OP.
Didn't address #3 yet.
Comment #8
Damien Tournoud CreditAttribution: Damien Tournoud commentedYeah. I was looking at an old checkout of 8.x.
Comment #9
xjmWhen I went to implement the first part of #3 (which should also fix one of the remaining test failures), I noticed that the result was that of replacing all core usage of
SCHEMA_INSTALLED
with N000.So maybe it makes sense to just re-
define()
SCHEMA_INSTALLED
with\Drupal::CORE_COMPATIBILITY * 1000
? Attached tries that. (No code for preventing updates yet, but I agree that should be in scope, so retitling accordingly.) NR for the bot.Comment #10
catchIsn't core compatibility '8.x' as a string?
Comment #11
xjmHaha, it totally is.
Maybe we should instead just replace
SCHEMA_INSTALLED
withDrupal::CORE_MINIMUM_SCHEMA_VERSION
or something then?Comment #12
xjmAttached does that instead (also fixes an error in the previous logic).
Comment #13
xjm@dawehner noticed that this is a little confusing with both
$systemv_versions
and$system_version
, so it'd probabl be clearer to just set $version = \Drupal.... first and then go from there.The fact that this is more or less a duplicate of the other hunk makes it look like it wants to be factored out, but possibly out of scope.
Waiting for the bot, though.
Comment #14
xjmWith update compatibility handling and some cleanup. Needs tests still though.
Comment #15
xjmAhem.
Comment #18
xjmAttached fixes some merge conflicts, plus:
hook_update_N()
documentation.hook_update_dependencies()
andhook_update_last_removed()
.update_prepare_d8_bootstrap()
to throw a requirements error for a D7 database.REQUIRED_D7_SCHEMA_VERSION
constant.update_fix_d8_requirements()
since it is no longer needed (D7 sites will never get pastupdate_prepare_d8_bootstrap()
).There's still a bit of documentation that needs updating around the update API function examples (we need to actually re-think the example code). There's also a bit in
UpdateModuleHandler::install()
that needs updating.FInally, we should probably remove all these unless the Migrate folks are using them:
That could probably be done in a followup issue.
Comment #20
xjmProbably should keep the second sentence of the original comment here.
Comment #21
xjmAttached fixes that comment (and a, er, syntax error).
Comment #23
Damien Tournoud CreditAttribution: Damien Tournoud commentedHehe. Sorry about CORE_COMPATIBILITY being a string :)
^ This is debatable. Do we want to just silently skip those modules or do we want to raise a warning like the handling of
update_last_removed
?Comment #24
xjmWell, the check against update_check_incompatibility() fails silently if the module's .info file refers to an older version of core or PHP, regardless of whether it was previously installed, so that's why I included it there. It seemed comparable. I could see raising a requirements warning too; I wasn't sure why that isn't already the behavior for installed modules that use an older version of core/PHP/etc. Followup maybe?
Here's a reviewable-sized patch for the functional changes up through #21, without the moved update hooks.
Comment #27
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe support for
update_last_removed
is pretty similar to what we have here: it's about a module that is compatible to the current Drupal core but has a schema version that is lower then the first update function available.It doesn't really matter, because this scenario should be impossible with Drupal 8, but it makes sense to me that those two have the same effect.
In other words, we have two different behaviors right now:
Comment #28
catchIt's possible to get into an update_last_removed pickle with a contrib module still. Say they have 8.x-1.x branches up to 8.x-7.x and upgrading from each requires going through each intermediate branch - then the 8.x-7.x could have an update_last_removed that is 8600, but your site is still on 8001 or whatever.
Comment #29
xjmAttached fixes a couple of dumb typos that broke
update.php
(good times) and also resets a static cache in a test that worked by accident before.The update dependency tests assume that the system module has at least three update hooks; they'll need to be reworked to rely on a different test module instead. For now I just stuck empty hooks in
system.install
to demonstrate that the patch works otherwise.Comment #30
xjmComment #31
xjmAttached fixes the update dependency tests to use another test module, and gleefully rips out the variable-to-config upgrade path, its tests, and other unused update code.
Comment #32
chx CreditAttribution: chx commentedSo, I am OK with this approach -- we can do git mining or we are grateful we don't need to, we will be all OK. The changes that usually enter into migration is usually some stupid functionality storing their configuration in variables in some weird ways, that's all we really need to read out.
However, it is time we actually mandate migrations from people who write such changes. If there are no update hooks we do need to begin doing that or we leave some data behind. Most often, it will be like "ah yeah, this change will be covered by the node/user/term etc migration".
Comment #36
chx CreditAttribution: chx commentedWe discussed process ongoing with xjm and got to the point where a new policy would be in place: File an issue in the core queue, migration component if you would add a hook_update_N() that actually interacts with site data rather than just the schema. We still will close most of those but this is the best we can come up with until people more understand what and how migrate is doing what it's doing.
Comment #38
xjmHappy green patch. Time to tag; it's getting close.
Updated the summary and remaining tasks.
Comment #41
xjmNW for the new tests and docs updates.
Comment #42
xjmAttached adds the missing test coverage and updates the remaining API documentation.
Comment #43
Damien Tournoud CreditAttribution: Damien Tournoud commentedAny words on the bottom half of #27?
Comment #45
xjmI still think somehow having a 7XXX schema installed on D8 is more like the first case, but I don't feel strongly about it. I guess I'd like to test it manually to see which felt more natural, but OTOH I can't imagine a scenario that could lead to it other than an API call manually changing the schema version. I guess informing the user is unlikely to be a bad thing.
Also realized over night that throwing an exception is probably too aggressive, especially here. It'd probably just be better to have a requirements warning on both installation and update, since this could make currently ported contrib modules explode sites, for example, and having that misnamed hook in the codebase is unlikely to break anything during normal site operation unless we make it.
Marking NW for those two changes.
Also does anyone know what the proper tag for migrate-related issues is? :P
Comment #46
xjmAttached makes those changes. I noticed a bug in the process of testing this: If there are only invalid updates,
update.php
does not display the error messages fromupdate_get_update_list()
, resulting in this confusing screen:This also means that
hook_update_last_removed()
has insufficient test coverage (and it actually appears "insufficient" is "none" in this case). This is out of scope, though, so I'll file it as a separate issue. For now the patch uses less specific, but still valid assertions to confirm that a requirement error was raised.Attached also fixes the comment in
UpdateModuleHandler
; @alexpott suggested leaving the support for stream wrappers and just making the comment generic. There's still this other bit inUpdateModuleHandler::install()
that doesn't make so much sense anymore:Comment #48
xjmChecked with @catch about #46 -- #1929816: Remove all references to/implementations of hook_schema_0() will take care of that, so we can exclude it from the scope here.
We could probably use someone other than me testing it manually, and someone testing with a D7 database to make sure it works as intended. Other than that, I think this patch is pretty much ready.
Comment #49
xjmNeeds a reroll already.
The -do-not-test patch shows all the changes except the actual moving of the update hooks and helpers to
old_updates.txt
, to make it easier to do a code review on the functional changes.Comment #51
webchickJess gave an overview of this patch today, and one thing I was surprised about is we keep this file "old_updates.txt" hanging around in the root /core directory. That file, which is really only for a small subset of the core developer team, might very well sit there for N months/years until the D7 -> D8 upgrade path is finished, i.e. we might very well ship Drupal 8.0.0 with it, and possibly 8.1.0 as well. In the meantime, it's front-and-center to anyone who's learning Drupal 8 for the first time, for them to be confused by.
It seems to me like a better thing to do would be to take that output and upload it to a meta issue that's about making sure all of the old update functions are reflected in the migration path, and/or turn it into a body text of said issue with <strike> in it or what have you. But putting it in the actual code base feels a bit ick to me. It would also make this patch much easier to review.
Comment #52
jessebeach CreditAttribution: jessebeach commentedI ran a 7.x to 8.x upgrade just to baseline. Those steps were:
/core/update.php
After the requirements check page, I get a page listing the number of pending updates. 169 updates are listed.
If I run the updates, the process finishes with numerous errors, but nothing fatal:
Comment #53
webchickJust spoke with chx about #51, and he's +1. Basically, what he can do is create a branch in the migrate sandbox that's from the commit before the commit of this patch for migrate volunteers to browse as a reference implementation.
Then they can do a one-time run-through of all of these at https://docs.google.com/a/acquia.com/spreadsheet/ccc?key=0AgTfFkG3LW9idG... in order to catalog what needs to be accounted for and where to find it.
So here's to < 400K patches in the future. :D
Comment #54
jessebeach CreditAttribution: jessebeach commentedIn
system.install
, do we want to delete this block of code, since we shouldn't even be getting this far in the update process?Comment #55
jessebeach CreditAttribution: jessebeach commentedI've been trying to upgrade from 7.x to 8.x with the patch in #49. I'm getting a WSOD and no error in the PHP log. I've widdled the exception down to this function and specifically, this line:
\Drupal::state()->set('system.module.files', $files);
insystem.module
.Comment #56
jessebeach CreditAttribution: jessebeach commentedIf I comment out the set state line mentioned in #55 above,
\Drupal::state()->set('system.module.files', $files);
then the request returns and I get a requirements page indicating that my DB is from Drupal 7 and I need to migrate to continue.
Comment #57
jessebeach CreditAttribution: jessebeach commentedOk, so my guess here is, we're running requirements checks, one of which --
system_requirements
-- tries to cache a list of files...which requires a database. And since we're moving from 7.x to 8.x, we don't have a valid DB in this case.Comment #58
jessebeach CreditAttribution: jessebeach commentedI've introduced a function
update_migrate_requirements
that does the 7.x to 8.x check. If that check return a requirement array, then we don't check any other requirements and instead skip right to printing out that one to the screen.Comment #59
xjmAwesome, thanks @jessebeach! That looks like more what was supposed to happen. ;)
Attached just rerolls #58 to remove
old_updates.txt
. This is now only maybe the fifth largest patch I've worked on in the past year and a half.Comment #63
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis is obviously a bug in
system_requirements()
: both $phase = 'install' and $phase = 'update' need to be safe to run with the minimal install / update environment.So this is just wrong and need to be fixed:
Comment #64
xjmRe: #63, isn't that out of scope here as an existing bug in HEAD, one that does not need to be resolved to fix this issue?
Comment #65
xjmFor reference, #63 refers to line 309 of
system.install
.Here's the same hunk in D7:
It used a variable then, too. Is the difference that
variable_get()
returns nothing when$conf
is not set, meaning the default would be used, but the D8 version has no such fallback?Comment #66
catchIt's not just the lack of fallback, it's that variable_get() has no inherent dependency on variable storage.
Which makes me wonder why these are calling out to config at all, should be settings after #1856766: Convert file_public_path to the new settings system (make it not configurable via UI or YAML) no?
Comment #67
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe problem is actually here:
It works in D7 because we have the requirements update running before running requirement checks.
Based on this, I'm going to agree with @jessebeach solution: we only run requirement checks if we are already on D7.
Comment #68
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis said, could we refactor this so that we don't have this weird indirect store/get pattern?
Comment #69
catchOpened #2170235: file_private_path should be in $settings, like file_public_path anyway.
Comment #70
jessebeach CreditAttribution: jessebeach commentedDamien, I'd love to get your opinion on best practice for a better pattern here. I'm not exactly an expert on PHP at this level.
Comment #71
chx CreditAttribution: chx commented> And since we're moving from 7.x to 8.x, we don't have a valid DB in this case.
What are we talking about? Did I miss something? You are not supposed to do that any more. You are supposed to install a fresh standard D8 and run migrate against the D7 database. If you point Drupal 8 to a Drupal 7 database it will work as well as if you pointed it to a phpBB database -- not at all.
Comment #72
jessebeach CreditAttribution: jessebeach commentedMore likely I missed something :) I was just making a guess as to why it was failing, but I don't know how to debug any further than I did.
Comment #73
xjmRight, this is what is supposed to happen (and now does happen) if you try to use a D7 database:
:)
(Though, I just noticed, we need to remove the "please" per our UI text guidelines.)
Comment #74
xjmThat's where. :)
Comment #75
jessebeach CreditAttribution: jessebeach commentedThis patch update addresses the refactor from #68 and the 'Please' from #73.
Comment #76
xjmNeeds a reroll; not applying.
Also, we should not use
drupal_static()
here. There's no reason that this should be clearable on a cache clear. I think what DamZ means is to make the code simpler -- I'd start by just inlining the contents of the function in the two places that it's called, and then simplify/refactor from there. I'll look at it again later today once I'm off the phone.Comment #79
amateescu CreditAttribution: amateescu commented@jessebeach, our "usual" store/get pattern is drupal_set_*()/drupal_get_*(); see for example
drupal_get_title()
anddrupal_set_title()
. I *think* that's what Damien meant in #68.In this case it would mean:
- rename
update_extra_requirements()
toupdate_set_extra_requirements()
- add a new
update_get_extra_requirements()
that just doesreturn update_set_extra_requirements();
- same for update_migrate_requirements()
Also, I'm not sure the change to use drupal_static() is needed, but if you want to keep it, you can pass a default value of array() as a second argument instead of those ifs.
Comment #80
xjmI really don't think we need so many helper functions for a few-line check, unless I'm missing something? Maybe I haven't understood completely. :) It doesn't seem like we ever need this as API outside this particular hunk of code.
Comment #81
jessebeach CreditAttribution: jessebeach commentedOk, I've rolled everyone's comments together in my brainmeat and I'm trying again.
Comment #82
jessebeach CreditAttribution: jessebeach commentedIt turns out that the two functions
update_extra_requirements
andupdate_migrate_requirements
were just wrapping one check each. So I've given each check its own function and just call them directly now fromupdate_check_requirements
.This patch is literally going stale as I'm writing it. Folks are committing hook_update functions at a hot clip.
Comment #83
webchickWait. Really? They better not be! :)
Comment #84
jessebeach CreditAttribution: jessebeach commentedSorry, the old_updates.txt snuck back in there. Removed. No other changes to the code.
Comment #85
jessebeach CreditAttribution: jessebeach commentedWe already had conflicts in locale.install and user.install in the past 24 hours.
Comment #86
Damien Tournoud CreditAttribution: Damien Tournoud commentedThanks for the simplified code flow. From a visual inspection, #84 looks ready to go.
Comment #87
Damien Tournoud CreditAttribution: Damien Tournoud commentedActually, this is really far from being ready.
We are still calling
update_prepare_d8_bootstrap()
early inupdate.php
. This fiddles with the Drupal 7 database and settings.php and it is the only reason we currently get far enough to display the error message.I can think of two ways to move from there:
drupal_bootstrap()
into a try/catch block and display a manual error message like we do ifversion_compare(PHP_VERSION, '5.3.10') < 0
;Comment #92
jessebeach CreditAttribution: jessebeach commentedI do not immediately know how to proceed with Damien's comments in #87. I am also looking at a couple other issues now as well. So in the interest of maintaining momentum here, Damien, would you be able to update the patch with your proposal? If not, I encourage others to attempt to incorporate this feedback into the existing patch. I will probably return to this issue in a few days, but I want to make sure that others aren't waiting on me to respond in the between time.
Comment #93
xjmI also do not understand #87. @DamZ, are you saying that we should remove
update_prepare_d8_bootstrap()
?Meanwhile, here's just a reroll. It applies to
2a228e60a
.Comment #94
Damien Tournoud CreditAttribution: Damien Tournoud commented@xjm: yes, we cannot ship with
update_prepare_d8_bootstrap()
, because it *modifies* your database and settings.php (among other things).Comment #95
catchJust this would be enough for me - as long as we give some kind of message and direct people where to go.
Comment #96
star-szrAdding a related major that maybe can be closed as a duplicate now?
Edit: Or at least it should be looked at, I found it digging around on the last page of major tasks.
Comment #97
xjm@Cottser, good find! I closed it as a duplicate.
Comment #98
jessebeach CreditAttribution: jessebeach commentedI've inserted a very simple check for <8000 schema version in the system table in update.inc
I'm fairly certain that this runs before we start manipulating the database. I've run this check as soon as is possible given that we need to run a
db_query
sodatabase.inc
needs to be loading and that file requires some configuration in order to establish a DB connection.I've left in the later requirements check for the schema during the requirements gathering phase. The most we can determine early on is if a
system
table exists and if it has aschema_version
field and if the value of this field is less than 8000. If none of that is true, then we don't necessarily have a Drupal 8 database; we just know we don't have a Drupal 7 database.I also added a test that mocks a Drupal 7 database and asserts that the error message is printed. The
-fail
patch file includes just this test.I think this should cover all the commentary so far.
Comment #100
Damien Tournoud CreditAttribution: Damien Tournoud commented@jessebeach Good! But the rest of update_prepare_d8_bootstrap() (modifying settings.php, installing the config directories, etc.) is unnecessary now and needs to go away too.
Comment #101
jessebeach CreditAttribution: jessebeach commentedAlright, this update removes the unnecessary manipulation of the settings file and config directories.
moshe suggests that we an go further with the refactoring of update.php, perhaps even turning it into a controller on a route rather than a top-level page that needs to manage a bootstrap.
I'm personally a little hesitant to go down that road in this issue especially since it feels like we're right about to resolve this critical. Refactor would be a very agreeable nice-to-have follow-up.
Comment #102
xjmAgreed, let's file a followup for that, since it should not block a beta.
Comment #103
webchickNo longer applies. :( Looks like #2121849: Move file_chmod_directory and file_chmod_file out of config and into settings so that drush_chmod() is independant of the config system is the culprit. It's only a major, but it does unblock Drush so I'd rather not roll it back.
Comment #104
XanoRe-roll because of system.install.
Comment #107
xjmCleaning up a couple other stale references. (The
UpdateModuleHandler
code will be removed in #1929816: Remove all references to/implementations of hook_schema_0(), but the reference is just confusing now so best to simply remove it.)I confirmed that all
hook_update_N()
for core are still removed, and all the remaining references to*_update_8NNN
andhook_update_N()
have updated documentation.Comment #108
xjmHere's
update_prepare_d8_bootstrap()
as of the current patch:I think we probably want to move or get rid of those last hunks as well, remove the function entirely, and move the try/catch up into
update.php
?Comment #109
xjmAttached moves most of what remained of
update_prepare_d8_bootstrap()
directly intoupdate.php
and removes a D7 upgrade-specific bit from #1886448: Rewrite the theme registry into a proper service. I think the rest is a required part of bootstrapping to run updates (and therefore I'm not entirely sure why it was inupdate_prepare_d8_bootstrap()
, but any further refactoring or cleanup needn't block a beta, I think).Comment #110
BerdirI think this would be easier to follow if the if below is within the try block, as you don't need the isset($system_schema) then and the code that belongs together is together.
That said, I'm wondering if a db_table_exists() + a db_query() would be better, to avoid exceptions for the expected code flow.
Comment #111
xjmMakes sense. Rerolling for that.
Comment #112
xjmComment #113
BerdirTiny nitpick: The isset() is no longer necessary, even if the query would not find anything, it's explicitly set to NULL and you can compare it.
Comment #115
xjmI tend toward cautious when it comes to loose typing, but:
So fixed in attached. :P
Comment #116
jessebeach CreditAttribution: jessebeach commentedThere was a stray comment that no longer applied. I removed it since the code is fairly self-explanatory here: load settings and bootstrap the kernel.
- // Now remove the cache override.
I read through the changes again and it's looking great. I think we're ready to get this in!
Comment #117
BerdirAlmost, needs a re-roll first, #2167109: Remove Variable subsystem got in and has a few places that conflict with this, although I tried to minimize the overlaps :)
Comment #118
znerol CreditAttribution: znerol commentedRerolled.
Comment #120
znerol CreditAttribution: znerol commentedInitialization of the request somehow got lost in the last reroll. Hope this will turn green again.
Comment #121
jessebeach CreditAttribution: jessebeach commentedUpdating from 7.x to 8.x is properly blocked.
Updating from 8.x^ to 8.x functions as expected.
I think we're ready to get this in. Any objections?
Comment #122
BerdirSee #2178581: Add an AnonymousUserSession object in favour of using drupal_anonymous_user(), what should we do about the generate-dX-content.sh scripts? I think they're kinda useless as migrate tests afaik work differently and don't need them and as assuming over there, they are very likely hopelessly broken anyway.
Comment #123
Berdir120: update-2168011-119.patch queued for re-testing.
Comment #124
jessebeach CreditAttribution: jessebeach commentedI can't speak to the veracity of this, but I trust berdir (and in general :) ) here. But the question for this issue is, can we cleave off a non-Critical sub-issue to deal with
generate-dX-content.sh
and get this Critical closed? If the feeling is no, then I'll look into berdir's comment.Comment #125
catchI think we can look at that in a follow-up.
Comment #126
xjmchx said those were useful for migrate, but that was awhile ago. And yes, let's open a followup for anything not in the current scope, please. :)
Comment #127
jessebeach CreditAttribution: jessebeach commentedMajor followup: #2185565: Figure out what to do with dump-database-d*.sh and generate-d*-content.sh scripts.
From #drupal-contrib:
So I've removed the generate scripts from
/core/scripts
.Alrighty then, I think we're good to go.
Comment #128
jessebeach CreditAttribution: jessebeach commentedAlso, the diff --stat, which I think is fun in this case :)
Comment #129
xjmUm, hang on. Let's please not introduce that change here; it's out of scope.
Comment #130
jessebeach CreditAttribution: jessebeach commentedOk.
I added mention of the
generate-d*-content.sh
scripts to #2185565: Figure out what to do with dump-database-d*.sh and generate-d*-content.sh scripts.Attached is the patch from #119.
Comment #131
webchickYeah, agreed. I'm not sure I agree that we can remove them. It's fine if Migrate doesn't use them, but we will still need test coverage of updates in core once we start adding them.
/me refreshes https://qa.drupal.org/pifr/test/717138 every 8 seconds. ;)
Comment #132
xjmFor code review sanity, here's a patch with only the functional changes, doc updates, and added/revised test coverage.
Comment #133
xjmNote that the function is moved, not removed.
Comment #134
xjmComment #135
xjmComment #136
catchI had this open in a tab and it wasn't RTBC or committed this morning, let's get it to RTBC.
Comment #137
jessebeach CreditAttribution: jessebeach commentedChange record draft: https://drupal.org/node/2186315
It's blank, but there's the stub. I can fill it out later this afternoon.
Comment #138
xjmWe need to replace this with a link to https://drupal.org/upgrade (or whatever) once that page is updated to actually reflect D8 migration stuff.
Comment #139
webchickAll right, awesome. Jess walked me through this patch for a good 30-40 minutes.
The one thing that did mess me up was the "you can't name an update function _8000 anymore" stuff. Because as a "consumer" of the upgrade API, I never realized that that was special in any way. It does make a lot of later checks a lot easier, though, so makes sense. Let's just be sure to call this out in the change notice very strongly, because that's going to mess some folks up.
I also spotted at least one place where we're giving an end-user facing message and linking off to dev docs. We need to not do that, and we need to make sure there's a place for those people to go, so we need a major follow-up for that.
Otherwise, this looks great, AWESOME work on this everyone!!
Committed and pushed to 8.x. WOOHOO!
Comment #140
xjmChange record published: https://drupal.org/node/2186315
Yay!
Comment #141
xjmFollowup #1: #2186531: Update handbook documentation on upgrade/migrate for D8!
Comment #142
xjmFollowup #2: #2186565: update.php does not display the error messages if there are only invalid updates
Comment #143
xjmAnd, finally, a g.d.o/core announcement on what to do instead of adding
hook_update_N()
to D8 patches: https://groups.drupal.org/node/402803Comment #144
hass CreditAttribution: hass commentedIs there any documentation what I need to do in a module to migrate variables? Cannot find anything helpful.
Comment #145
tim.plunkettSee core/modules/migrate_drupal/config/migrate.migration.d6_node_settings.yml for an example. That takes the variable node_admin_theme and makes it the use_admin_theme key in the node.settings.yml file.
Comment #146
xjmMigration API docs: https://drupal.org/node/2127611
Comment #147
hass CreditAttribution: hass commentedThanks tim, will look into it.
@Xjm: i found this and have not understood anything. There are zero examples or I'm too stupid to understand them. This is not a migration guide and the changelog does not contain any instructions. This should be changed.
Comment #148
webchickI think https://drupal.org/node/2141805 is what's being asked for here. It's possible this should be combined, or at least cross-linked, to the change notice for this issue.
Comment #149
hass CreditAttribution: hass commentedYeah, thats a lot better. However I'm missing where I need to save the migration files in my module and I'm confused by these D6 naming... There is no D7? Is the d7 migration not yet in core?
Comment #150
xjm@hass, maybe you could help make the API doc page better based on https://drupal.org/node/2141805 and the example @tim.plunkett gave?
And yes, the D6 migration is being developed first, then the D7. (Neither are complete yet.)
I updated https://drupal.org/node/2186315.
Comment #151
alexpottCreated followup #2190695: Remove unnecessary update functions
Comment #152
chx CreditAttribution: chx commentedWe have an API documentation but not a howto (yet?). While we do not really have a good variable-to-config migration guide written we do have one for entities https://groups.drupal.org/node/387488 which describes in painstaking detail on where to put a YAML and so on. Together with the gazillion existing variable-to-config migrations this I hope will be helpful.
Comment #154
KyleNakhul CreditAttribution: KyleNakhul commentedCan someone there help how to address this issue. I am trying to upgrade from version 8.21 to 8.2.4 and I'm the following error message when running www.mydomain.com/update.php. It's on a live server and I can't use Drush. Any help will be highly appreciated.
Here is the error