Creating a content type on hook_update() fails with Fatal error: field_attach_create_bundle() in node.module on line 593:

http://skitch.com/alexbarth/bcmg9/updating-d7agtest

Creating a content type on hook_enable() works fine.

Will post a patch soon.

CommentFileSizeAuthor
#14 439236-14_field_load_during_update.patch1.21 KBalex_b
#10 field-load-during-update-439236-10.patch1.57 KBbjaspan
#1 439236_ensure_field_attach_create_bundle.patch746 bytesalex_b

Comments

alex_b’s picture

alex_b commented
Assigned: alex_b » Unassigned
Status: Active » Needs review
StatusFileSize
new 439236_ensure_field_attach_create_bundle.patch746 bytes

Wrapping the field_attach_create_bundle call in node_type_save() fixes the error.

I am not sure whether this is the right level to address this issue though.

bjaspan’s picture

bjaspan commented
Status: Needs review » Needs work

This patch is not the right approach. field_attach_create_bundle() is loaded by field_init(), so the question is why field_init() has not run.

I see that the problem occurs during an update function. What module is updating? Is this part of a D6 to D7 upgrade, or is the site already fully D7?

alex_b’s picture

alex_b commented

#2:
It occurs in a D7 to D7 upgrade. #293318: Convert Aggregator feeds into entities introduces feeds as nodes, hook_update() adds a default feed content type when upgrading. Eventually, most people will upgrade D6 to D7 (I haven't tested this yet).

This is the particular patch that I applied that caused this bug to surface: http://drupal.org/node/293318#comment-1497410 (installed aggregator, applied patch, ran upgrade scripts).

alex_b’s picture

alex_b commented

I did some more digging.

hook_init() is not invoked on update.php. This means that functions loaded in field_init() aren't available on updates.

Not sure what's the best way to solve this issue.

For details see:

update.php:

/**
 * Global flag to identify update.php run, and so avoid various unwanted
 * operations, such as hook_init() and hook_exit() invokes, css/js preprocessing
 * and translation, and solve some theming issues. This flag is checked on several
 * places in Drupal code (not just update.php).
 */
define('MAINTENANCE_MODE', 'update');

common.inc:

function _drupal_bootstrap_full() {

  //...

  // Let all modules take action before menu system handles the request
  // We do not want this while running update.php.
  if (!defined('MAINTENANCE_MODE') || MAINTENANCE_MODE != 'update') {
    module_invoke_all('init');
  }
}
bjaspan’s picture

bjaspan commented

Good digging. Now we need to find out why hook_init is not called during update.php. Let's see if we can ask around for commentary from some of the Elder Ones. :-)

damien tournoud’s picture

damien tournoud commented

That hook_init() is not called on update is a bug, but the major bug is that the Field API uses hook_init() to include code while we now have a full-featured registry.

Related work: #445044: Try to remove the field autoload "feature".

bjaspan’s picture

bjaspan commented

cvs annotate to the rescue.

The decision not to invoke hook_init during update came from #179143: Do not fire bootstrap hooks during update. I did not read the entire issue so I do not yet fully understand why, though it seems that even at the time people were expressing reservations about it. I'll ping some of those people to see if they can chime in here.

moshe weitzman’s picture

moshe weitzman commented

The justification for suppressing hook_init() starts at http://drupal.org/node/179143#comment-625762. But that problem is specific to the D5 => D6 major upgrade. Tossing hook_link() just for that one time problem seems excessive to me - we use update.php for every contrib minor upgrade, which is to say we use it a lot.

Except for that major upgrade, it will be safe to do anything you ant in hook_init() - we have just done a drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);.

Personally, I think we can revert that part for D7.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

The less code we run on major upgrades, the better. And we are unfortunately not sure whether people upgrade from 6.0 to 7.0 or 6.11 to 7.5. So "except for the major upgrade" seems like non-trivial to implement (unless we just depend on the update function numbering or somesuch, but then update functions should be conditional on what kind of update is happening, ugh).

bjaspan’s picture

bjaspan commented
Status: Needs work » Needs review
StatusFileSize
new field-load-during-update-439236-10.patch1.57 KB

Okay, so how about not using hook_init() at all and just explicitly including the required files whenever field.module is loaded?

alex_b’s picture

alex_b commented
Status: Needs review » Reviewed & tested by the community

#10 fixes the problem I described in my initial post.

The approach seems sound, RTBC from my point of view.

moshe weitzman’s picture

moshe weitzman commented

Good enough for now. RTBC from me too.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

alex_b’s picture

alex_b commented
Status: Needs work » Needs review
StatusFileSize
new 439236-14_field_load_during_update.patch1.21 KB

Resolved conflicts and rerolled.

alex_b’s picture

alex_b commented
Status: Needs review » Reviewed & tested by the community

#14 merely resolves a conflict. Setting back to RTBC.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +Fields in Core

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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