On June 29, following the release of Drupal 8 beta 12, Drupal 8 core patches that include data model changes must include a hook_update_N() implementation and test coverage for it.

What additional steps must be taken for core issues after June 29?

  1. Data model changes should be documented in the issue summary.
     
  2. Any patch that introduces a data model change must include both a hook_update_N() implementation and a corresponding update path test it in order to be committed after June 29. In most cases, the update should also be tested manually on a test site. (See the head2head project for examples of hook_update_N() for Drupal 8 betas.)
     
  3. Critical issues that require a difficult update function may have a critical followup to provide the update function and test if it would significantly delay the resolution of the critical otherwise. Note that existing critical issue #2453153: Node revisions cannot be reverted per translation is still outstanding with a potentially significant data model change, and may be exempted from this policy even if it is fixed after June 29.
     
  4. Additionally, any issue that requires a .htaccess, web.config, settings.php, or services.yml update should potentially include a change record. (In later betas, these issues will also be mentioned in release notes, or potentially will not be allowed if it's a minor change that would cause instability for existing installs. See #2341575: [meta] Provide a beta to beta/rc upgrade path.) Also see related issue #2507509: Service changes should not result in fatal errors between patch or minor releases.

What is a data model change?

A data model change is any change that makes stored data on an existing site incompatible with that site's updated codebase. In Drupal 8, this includes:

Database schema changes

Any change to a hook_schema(), including:

  • Adding, changing, or removing a database table or field.
  • Moving stored data to different fields or tables.
  • Changing the format of stored data (for example, changing the stored format of user-entered paths, requiring a different password hashing algorithm, or storing a UUID as an external key instead of a serial ID).
  • Etc.

Note that in Drupal 8, content entities and fields support automatic updates to database tables when entity and field schema definitions change. Any update to a content entity or field should be manually tested to ensure the automatic update is completely functional. Some changes may still require update functions.

Configuration data model changes

  • Removing or renaming a configuration key.
  • Adding a configuration key. (Even if the additions are backwards-compatible in terms of stored data, they will introduce unexpected differences in exported configuration the next time the configuration is saved.)
  • Changing the expected data type, allowable values, or array structure of a configuration key.
  • Changing the expected default value of a configuration key.
  • Changes to configuration dependencies (e.g. changes to a plugin's dependencies or an implementation of ConfigEntityInterface::calculateDependencies()).
  • Etc.

For example, changing the way a particular Views plugin defines its configuration would require an update to all saved views that use that plugin.

How can I help?

Note for Drupal site owners and developers

Betas are good testing targets for developers and site builders who are comfortable reporting (and where possible, fixing) their own bugs. Betas are not supported releases of Drupal, and generally are not recommended for non-technical users, nor for production websites. The current beta release includes known critical bugs, including publicly disclosed security vulnerabilities. More information on beta releases.

The addition of update functions for Drupal 8.0.x issues should make it easier for developers to update their existing development sites between beta releases using update.php. However, these update functions may include bugs or even introduce data integrity issues, so developers should always back up the site and be prepared to rebuild it from scratch or manually migrate the data in case update.php fails to update the site data properly.

Comments

xjm’s picture

Title: [policy, no patch] hook_update_N() required for Drupal 8 core patches beginning June 24 » [policy, no patch] Require hook_update_N() for Drupal 8 core patches beginning June 24
catch’s picture

Changing the default configuration provided with a module or profile.

This might need dividing up.

If we're changing a default value from one existing option to another, we might want to leave existing sites as they are, and not provide an upgrade path.

There's no way to tell if someone has submitted a configuration form with that default or not - so we can't tell if they don't care, or have explicitly left a default as-is.

Any other shipped configuration change should get an upgrade path though.

xjm’s picture

From @catch:

Changing the default configuration provided with a module or profile.

This is 50/50. Sometimes we might change the default and want to update existing sites. Sometimes we might want to leave the existing sites on the old default in case they've explicitly decided to use the default.

Edit: just see @catch's comment above. :)

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
pfrenssen’s picture

Oh my oh my! :)

stella’s picture

whohoo!

dawehner’s picture

Its great that we have formalized exactly what should be done.

Any changes to configuration schema metadata, including adding keys, removing keys, changing a key's type, or otherwise changing the structure. For example, changing the way a particular Views plugin stores its data would require an update to the configuration schema and an update to all saved views.
Changing the default configuration provided with a module or profile.
Etc.

So I'm curious why adding is a problem, given that config schema is optional. Just to be clear, nothing, at least in views, relies on configuration be actually complete. I think this assumption is also built into config schema. Schema is applied onto the actual configuration and is not started from the data model and being applied to the config.

In general I think reality will still show us surprises. They might range from simple things to the problem that our upgrade path might just work for core, but have issues with real sites.
Given that we should better push the changes now, so we gain that kind of knowledge rather sooner than later.

alexpott’s picture

Changing the default configuration provided with a module or profile.

I'm not sure that this is a data model change. A definitely should not require a hook_update_N change. It might require the module author to store the old configuration somewhere depending on the outcome of #2428045: Comparing active storage to install storage is problematic, install storage may change anytime.

re #10 as @dawehner says schema additions should not be a problem. Config key change and removal will always require a hook_update_N.

As @dawehner writes

In general I think reality will still show us surprises.

I think one area that will is how plugin's and config interact.

xjm’s picture

Issue summary: View changes

Updating the configuration section based on discussion with alexpott:

  • Removing or renaming a configuration key.
  • Adding a configuration key. (Even if the additions are backwards-compatible in terms of stored data, they will introduce unexpected differences in exported configuration the next time the configuration is saved.)
  • Changing the expected data type, allowable values, or array structure of a configuration key.
  • Changing the expected default value of a configuration key.
  • Changes to configuration dependencies (e.g. changes to a plugin's dependencies or an implementation of ConfigEntityInterface::calculateDependencies()

For example, changing the way a particular Views plugin defines its configuration would require an update to all saved views that use that plugin.

I removed any specific mention of schemata or default config since the required changes to these should be implied from the points above when relevant.

xjm’s picture

In general I think reality will still show us surprises. They might range from simple things to the problem that our upgrade path might just work for core, but have issues with real sites.
Given that we should better push the changes now, so we gain that kind of knowledge rather sooner than later.

I totally agree. Thanks @dawehner.

xjm’s picture

Issue summary: View changes
mpdonadio’s picture

Can we identify a few individuals to help assist in deciding whether an upgrade path is needed, and can also provide guidance with upgrade paths and upgrade path tests, or possible to start triaging issues that will need them?

I am not saying that this policy is bad, but I think we are going to see a whole bunch of major issues getting stalled because the full knowledge of how to proceed is going to be limited to a few people (who are already stretched pretty thin) until we get more upgrade paths and and upgrade path tests written.

Off to look at my issues, at least two of which are probably affected by this...

David_Rothstein’s picture

Does this mean the goal would be to have a complete/supported upgrade path between beta 12 and 13? What happens if the remaining upgrade path blockers mentioned in #2341575: [meta] Provide a beta to beta/rc upgrade path aren't fixed by then?

catch’s picture

@David iirc from the discussion that led to the issue summary, this is to have a 'practice run' for providing upgrade paths, in the expectation we'll find additional upgrade path blockers just by trying to write them. Also as @mpdonadio points out our collective knowledge and documentation on core upgrade path writing/testing is very thin and that's unlikely to improve.

So there will be no expectation that you'll be able to smoothly update from beta 12 to beta 13, this is more about getting used to providing updates for individual changes so that we can actually make a decent effort at smoother updates from then on.

Critical issues that need a hook_update_N() can have that opened as a separate critical issue, and at least between beta 12 and 13 we may end up won't fixing update-only that are particularly difficult to write updates for (head2head can still try to provide something). Or alternatively roll back the issue that required the update, then re-commit it after the next beta to give another month to get the update written. See the note about exceptions in the issue summary.

https://www.drupal.org/project/issues/search/drupal?project_issue_follow... has two issues (revisions and path aliases) that will very likely require proper data migration of content in, which is something I really want to avoid committing core to supporting. For that reason I'd personally strongly consider pushing this back a week to give us time to get those in. Then there's more chance of beta 12 to beta 13 updates actually working, which could mean we'd retrospectively support it.

The block config issue is one where the update path should be straightforward, and the service container issue is more of an 'update infrastructure' issue in that I don't think it will be possible to install beta X, put beta Y on top of it, then visit update.php until that is fixed. Slightly less concerned about those than the previous two in terms of this change.

In general, the list of major issues tagged with 'D8 upgrade path' is both short and not particularly scary looking (https://www.drupal.org/project/issues/search/drupal?project_issue_follow...), but I suspect that's because the bulk of major issues aren't properly tagged which is more scary...

@mpdonadio it would be great if you could triage your issues which are likely to be affected, make sure they're tagged and possibly post back here so we can look at them as examples.

David_Rothstein’s picture

@catch, thanks for the update.

For that reason I'd personally strongly consider pushing this back a week to give us time to get those in. Then there's more chance of beta 12 to beta 13 updates actually working, which could mean we'd retrospectively support it.

That sounds like a good idea.

I guess my main point is that if the release of beta12 marks the point in time at which updates need to be written, then I think the stated goal should be to provide a working update path from beta 12 to a future version (doesn't necessarily have to be beta13, I suppose). Is that the goal, at least?

I believe the point of writing update functions before an official release (before 8.0.0) is to encourage people to install more realistic development sites on Drupal 8 (that they won't have to throw away later), so that it can be tested on those more realistic sites before release. I am not sure why I'd want to spend time writing update functions if they aren't going to be used for furthering that purpose.

this is to have a 'practice run' for providing upgrade paths, in the expectation we'll find additional upgrade path blockers just by trying to write them

If an update path blocker is found while trying to write an update function then couldn't the issue that runs into it just be postponed until the more general update path blocker is fixed? In other words, I don't see why a "practice" period is necessarily needed to ferret those out. Have we ever run into a problem with this before?

mpdonadio’s picture

One of the issues I was worried about is #2399211: Support all options from views fields in DateTime formatters. I don't think this will need an upgrade hook. The patch doesn't add any new configuration or change schema. The old formatters aren't going away new ones are added. No settings are removed or changed, but some new ones are added. A container rebuild is necessary, though, for the new formatters to appear. I tested this for a while, and was unable to make it fatal without one, so I think a change record is all that is needed. I'll draft one, which can be used as a model for others.

The other is #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter. I am a little unsure about this one. When the timestamp formatters are ripped out of this, all that is left are changes to YAML configutation for Views and a few tests. And since we are keeping the views\field\Date plugin, existing views won't break, but new ones from a wizard or a module that gets enabled will use the new config from the YAML. So, I don't think this will need an upgrade path, but I will do some manual testing as a precaution.

As a side note, would it be beneficial to add

function system_update_8012(&$sandbox) {
}

to force the warning to users will run update.php, which will rebuild the container, etc, even if there are no changes, at least until RC1?

catch’s picture

The container rebuild issue should be fixed at least for core updates by #2507509: Service changes should not result in fatal errors between patch or minor releases.

xjm’s picture

While I think pushing the beta back another week to July 1 would probably be okay, I have two concerns: These various issues won't necessarily land by then anyway, and I'd also rather have lead time where we're requiring update hooks between now and the criticals sprint (which starts July 2). But I'm not strongly against it either, just so long as it's July 1 at the latest and we don't push it back again even if those issues don't land between now and then.

DamienMcKenna’s picture

It might be a silly question, but do D8 beta releases have to be on Wednesdays?

edit

Sorry for not finishing the train of thought.

If there is a very short list of items that the Powers That Be (people with commit access) really want in the next beta, could we compile the short list and get people to work on just these items? So then maybe the list is finished on Friday, or Monday June 29th, or July 2nd, but either way that list is kept short and worked on until they're done? Maybe also have an agreement that if one of the issues is in danger of disappearing down a rabbit hole that it gets bumped from the list?

alexpott’s picture

Additionally, any issue that requires a service container rebuild

- this seems a direct opposite of #2507509: Service changes should not result in fatal errors between patch or minor releases - are we sure we want to list that here?

xjm’s picture

@alexpott, that's from the main meta in #2341575: [meta] Provide a beta to beta/rc upgrade path.

xjm’s picture

Title: [policy, no patch] Require hook_update_N() for Drupal 8 core patches beginning June 24 » [policy, no patch] Require hook_update_N() for Drupal 8 core patches beginning July 1

Oh, and @DamienMcKenna, no, betas don't have to be on Wednesdays -- it's just a time that works for @catch usually and as a side benefit it's more consistent with D6/7 dates. But we move them around as needed.

I've updated the issue title and I'll post a quick announcement that we've moved the beta to July 1. Thanks everyone!

xjm’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes

Now that #2507509: Service changes should not result in fatal errors between patch or minor releases has landed I think we can remove the CR requirement for service container rebuilds.

DamienMcKenna’s picture

@xjm: Thanks for the explanation :)

xjm’s picture

Status: Active » Reviewed & tested by the community

I checked with @catch and we think this can be RTBC now.

xjm’s picture

Issue summary: View changes
xjm’s picture

Title: [policy, no patch] Require hook_update_N() for Drupal 8 core patches beginning July 1 » [policy, no patch] Require hook_update_N() for Drupal 8 core patches beginning June 29
Issue summary: View changes

/me should have put the date in a protected property

catch’s picture

There's an additional complication splitting the upgrade paths from the critical issues that introduce the schema changes, it's workable but noting it here.

Taking #2509300: Path alias UI allows node/1 and /node/1 as system path then fatals as an example because I was just in the process of typing a comment asking for the upgrade path to be split out.

Site A is installed using beta 12.

We commit #2509300: Path alias UI allows node/1 and /node/1 as system path then fatals. There is no upgrade path added.

Site B is installed.

Then we commit the upgrade path for #2509300: Path alias UI allows node/1 and /node/1 as system path then fatals.

Site C is installed.

Beta 13 is released.

Site A successfully upgrades from beta 12 to beta 13 using the upgrade path, that's fine.

Site C is installed after the upgrade path is added, so it doesn't run - that's fine too.

Site B has the correct schema already, but because it was installed before the upgrade path was added, the upgrade path runs.

We don't want to write upgrade paths that try to take into account the tiny minority of sites like Site B, because that's where we get into Drupal 7 hell, and we only 'support' beta to betas anyway.

My first idea is that when we defer upgrade paths for patches, we commit an empty hook_update_N() as a placeholder. Then the patch adding the upgrade path fills it out.

This way Site B's schema version would be correct, and Site A still gets the upgrade path run.

The other way is we completely ignore this case altogether and you have to manually add early return; to hook_update_N() you don't want to run if you start on a dev version then move to a beta later.

Fabianx’s picture

#32: Empty hook_update_N() sounds good to me.

Only we run into number commit conflict hell then ...

So either:

The real hook_update_N() number is added by the core committer and we use issue_123456() as functions in a helper file per module ...

Or we use something like drush update modules that also works for update.php:

Like core/updates/beta12/0001-issue-123456.php and track ran updates in some table manually.

And then have one hook_update_N() per beta version.

dawehner’s picture

#32: Empty hook_update_N() sounds good to me.

Well, I doubt its much easier, given that we have to sync those between issues anyway as well.

So pratically speaking for the purposes of testing we should have one meta per beta/RC upgrade path.
In there we a) collect the actual issues together with the upgrade path patches needed for a particular release.

This also gives us a nice central place to test upgrade paths for actual sites.

dawehner’s picture

Like core/updates/beta12/0001-issue-123456.php and track ran updates in some table manually.

Well, can't we achieve pretty much the same using such a naming convention for our update functions and get rid of using increasing versioning?
In general though this would be an API change ans so again increase the time until we can deal with upgrade paths, which is, kinda really important to do.

plach’s picture

Quoting from #2453153-126: Node revisions cannot be reverted per translation and below:

@effulgentsia

[...] or else we'd need a hook_update_N() that loads each entity, gives the field a value that is correct for that entity, and saves that entity.

@plach

Ideally I would like this to be fixed before supporting the upgrade path: you are right the schema would be updated automatically, but updating data would be tricky because we cannot rely on the Entity API in update functions and thus we could provide an update function only for the default SQL storage.

It's not clear to me whether our official policy wrt content entity field data is supporting only our default SQL storage, or whether we should assume using the Entity API is safe (which is definitely a bold assumption).

dawehner’s picture

Should this be now marked as "fixed"?

I'm curious mainly when #2507899: [policy, no patch] Require hook_update_N() for Drupal 8 core patches beginning June 29 will be marked as fixed ...

webchick’s picture

IMO we need a handbook page with the details from the issue summary about "When do I need an upgrade path?"

webchick’s picture

Hm. Probably also the missing "how to?" docs mentioned in the issue summary as well.

xjm’s picture

Status: Reviewed & tested by the community » Fixed

Several people were confused as to whether or not this change actually happened, because the issue was not marked fixed. So I've spun the issue for the docs changes out into a followup: #2521776: Update documentation for hook_update_N() for Drupal 8

Thanks everyone!

lauriii’s picture

Thanks for marking the issue as fixed! Makes it a lot more clear whether we should follow this or not.

webchick’s picture

We still need this policy of when we need an update hook documented somewhere less ephemeral than issue # whatever.

Not touching the status for now, but it seems like part of the "docs gate" related to resolving this issue.

xjm’s picture

@webchick, that's what #2521776: Update documentation for hook_update_N() for Drupal 8 is for -- feel free to expand the scope if needed?

webchick’s picture

Title: [policy, no patch] Require hook_update_N() for Drupal 8 core patches beginning June 29 » [policy accepted, needs docs] Require hook_update_N() for Drupal 8 core patches beginning June 29
Status: Fixed » Active

No, this has nothing to with "how to" write update hooks/tests. This is the policy in D8 on "when to" write update hooks in D8 that's captured in the issue summary here and nowhere else.

Let's try this.

Should be easy to put away. I can try do to so myself later this week if no one else beats me to it.

webchick’s picture

Title: [policy accepted, needs docs] Require hook_update_N() for Drupal 8 core patches beginning June 29 » policy, no patch] Require hook_update_N() for Drupal 8 core patches beginning June 29
Status: Active » Fixed

After talking with jhodgdon, she is going to roll the required docs from this issue into her D8 Accelerate grant for #2521776: Update documentation for hook_update_N() for Drupal 8.

Therefore, restoring status.

webchick’s picture

Title: policy, no patch] Require hook_update_N() for Drupal 8 core patches beginning June 29 » [policy, no patch] Require hook_update_N() for Drupal 8 core patches beginning June 29

...

Status: Fixed » Closed (fixed)

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