As the title states, the info_array in node_types_build gets rewritten for each module_invoke. Please correct me if i'm wrong, but I tried creating two modules with hook_node_info and the first content type always gets disabled on clear cache. Dsm revealed that the $info_array does indeed pose a problem when it checks for isset($info_array[$type_db]), so it disables the content type. Here's a small patch that fixes the problem.

CommentFileSizeAuthor
#21 986296-21-node-type-status.patch4.34 KBcarlos8f
#14 986296-14-node-type-status-TESTS-ONLY.patch3.25 KBcarlos8f
#14 986296-14-node-type-status.patch4.33 KBcarlos8f
#8 986296-8.patch1.21 KBtstoeckler
#3 node_types_build.patch613 bytesblup
node-types-build.patch589 bytesblup

Comments

blup’s picture

blup commented
Status: Active » Needs review

I meant "checks for empty($info_array[$type_db])". Also changing status.

Status: Needs review » Needs work

The last submitted patch, node-types-build.patch, failed testing.

blup’s picture

blup commented
Status: Needs work » Needs review
StatusFileSize
new node_types_build.patch613 bytes

Trying again from the root directory.

blup’s picture

blup commented
Version: 7.0-rc1 » 7.x-dev
blup’s picture

blup commented

Hmm, am i the only one concerned about this? This seems like quite a serious issue. Here's the commented excerpt:

  foreach (module_implements('node_info') as $module) {
    $info_array = module_invoke($module, 'node_info');        
    // Here the info_array gets declared in a foreach loop.
    foreach ($info_array as $type => $info) {      
      $info['type'] = $type;
      $_node_types->types[$type] = node_type_set_defaults($info);
      $_node_types->types[$type]->module = $module;
      $_node_types->names[$type] = $info['name'];
    }
  }
  $query = db_select('node_type', 'nt')
    ->addTag('translatable')
    ->addTag('node_type_access')
    ->fields('nt')
    ->orderBy('nt.type', 'ASC');
  if (!$rebuild) {
    $query->condition('disabled', 0);
  }
  foreach ($query->execute() as $type_object) {
    $type_db = $type_object->type;
    $disabled = $type_object->disabled;
    if (isset($type_object->base) && $type_object->base != 'node_content' && empty($info_array[$type_db])) {
    // And here it gets checked for ALL node types. However, only the last module's node types are present.  
    // Thus, the earlier node types get disabled.
      $type_object->disabled = TRUE;
    }
damien tournoud’s picture

damien tournoud commented
Priority: Major » Critical

Ew. This makes impossible for any module to implement hook_node_info(). Bumping to critical.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Needs work

Powered by Dreditor.

+++ modules/node/node.module	2010-12-01 17:05:18.104523996 +0100
@@ -686,8 +686,9 @@ function _node_types_build($rebuild = FA
+    $info_array += module_invoke($module, 'node_info');
     foreach ($info_array as $type => $info) {

This means we would be iterating over the same node types over and over again.
The second foreach should be taken out of the first one.

Powered by Dreditor.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review
StatusFileSize
new 986296-8.patch1.21 KB

Something like this.

Status: Needs review » Needs work

The last submitted patch, 986296-8.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review

JavaScriptTestCase? Let's try it one more time.

#8: 986296-8.patch queued for re-testing.

damien tournoud’s picture

damien tournoud commented

The only thing to fix is this line:

   if (isset($type_object->base) && $type_object->base != 'node_content' && empty($info_array[$type_db])) {

We should have a empy{$_node_types->names[$type_db]) here.

dmitrig01’s picture

dmitrig01 commented

What about adding a test here that enables blog and poll modules and ensure both work?

carlos8f’s picture

carlos8f commented

I agree with #11. $info_array is just a temporary variable, and shouldn't be used in the empty() check later.

And another issue I recently found: _node_types_build() also has a static cache that is a little too sticky. #993992: _node_types_build() labels content types 'is_new' when they're not, and has static caching bugs

carlos8f’s picture

carlos8f commented
Title: The info_array in node_types_build gets rewritten for each module_invoke » _node_types_build() accidentally marks node types as disabled
StatusFileSize
new 986296-14-node-type-status.patch4.33 KB
new 986296-14-node-type-status-TESTS-ONLY.patch3.25 KB

Quickie patch and tests.

Status: Needs review » Needs work

The last submitted patch, 986296-14-node-type-status.patch, failed testing.

int’s picture

int commented
Status: Needs work » Needs review

#14: 986296-14-node-type-status.patch queued for re-testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

+ $this->assertTrue(isset($types['book']) && empty($types['book']->disabled), t("Book module's node type disabled."));
The assert message should be "Book module's node type still active."
+ // Re-enabe the modules and verify that the types are active again.
Typo: Re-enabe -> Re-enable

Also thanks @Damien Tournoud for seeing the simple (and correct solution) over the complicated one :)

This is a critical, and the only problems I found are trivial documentation typos. Otherwise the code looks good, is a much better fix than the previous patches, and the tests prove it works (and, well, it comes with tests). This is RTBC for me, but I'll let this get another review first.

carlos8f’s picture

carlos8f commented
StatusFileSize
new 986296-21-node-type-status.patch4.34 KB

Thanks for catching those typos.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Thanks. Looks great! Setting 0.5 RTBC.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Needs review » Reviewed & tested by the community

Patch looks good. Tests are clean.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Oh boy! 2-line critical bug fixes with 30 lines of tests. My favourite! :D

Committed to HEAD. Thanks!

Status: Fixed » Closed (fixed)

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

pillarsdotnet’s picture

pillarsdotnet commented

Marked #943588: node_type_delete(): hook_node_type_delete will fail for a disabled node_type. Trying to get property of non-object in hook_node_type_delete() as a duplicate of this issue.