Disabled Node Example module still permits construction of Node Example content

I think the base==node_content is a terrible choice for a formalized example. The point of the example is to demonstrate building a node type inside a module. hook_node_info() or node_type_save()--either case is just as good, but imitating the behavior of a type built within the UI with base==node_content? Craziness.

How about a node type constructed by hook_node_info() with a name chosen to reflect the construction method, and a second node type constructed by node_type_save(), again, named to reflect its construction method. The overlap in the two work flows could be emphasized by combining data structures and using a single loop to create fields, attach fields, etc.

What do y'all think?

Sorry about that webchick. I thought that the node module was buggy based on the Node Example module's behavior, but it turns out that the Node Example was implemented as though it was built by UI.

@Mile23: I can't vouch for that patch just yet, but if you plan on updating the node example to base!=node_content, you should probably work against it (or a spec if you manage to find one). I can't find a spec, so I've been documenting against the available tests, which thanks to this headache seem to cover the problem pretty well. Looking around the bug reports I anticipate caching problems, but one problem at a time, yeah. Oh, and between all of the squabling over here, #1210092: PDOException: SQLSTATE[HY000]: General error: 1 too many SQL variables: SELECT t.* FROM {field_data_body} t WHERE (entity_type =, I drew the conclusion that maybe I should remove nodes in fixed size batches:

...
  // Balance database queries against memory use and PDO limitations
  $sql = 'SELECT nid from {node} n WHERE n.type = :type';
  $result = db_query($sql, array(':type' => 'iw_job_listing'));
  $count = 0;
  $nids = array();
  foreach ($result as $row) {
    $nids[] = $row->nid;
    $count += 1;
    if ($count >= 50) { // should test under a variety of environments
      node_delete_multiple($nids);
      $nids = array();
      $count = 0;
    }
  }
  node_delete_multiple($nids); // clobber the remainder
...

This sort of thing should be in the node_delete_multiple() I imagine, but I guess it's some kind of sensitive issue. Maybe this isn't an issue, though...I couldn't bring myself to pay much attention.

Glad to get some feedback. I was beginning to think that I was alone in the wilderness.

I was fishing in here to see if anyone was interested in the end result, not in doing the actual coding--I had planned on doing that myself (I'll probably have something Friday or so). I have something that can migrate with minimal pain (I used the blog module amongst others to figure some of this out). The code in core now is flawed, though, so the patch will be built on top of fixing _node_types_rebuild().

Personally, I find the API poorly documented (and disfunctional in my narrow experience building my first node type), but I don't mind as long as there exist modules to trace. Problem is, everyone seems to have hacked around a flaw in the node api's implementation, instead of fixing the problem. I love the example modules. Modules built to demonstrate functional workflows.

The blog module doesn't exhibit this bug because it doesn't remove its node type at uninstall. When uninstalling a module, the system claims, "The following modules will be completely uninstalled from your site, and all data from these modules will be lost!" I guess removing node_type_delete() from the API and altering that claim would as least be consistent, but I'm betting that the dev's ideal implementation included cleaning up after itself on uninstall. I've even found people discussing temporarily enabling a module at uninstall to work around this bug. I don't think that it's possible to fix the example module to something reasonable without first fixing _node_type_rebuild().

Oh, and st() is a static version of the t() that is available earlier in the bootstrap process, right?

As an aside, I was thinking about extending hook_uninstall() to let admins choose amongst several policies by radio button or something. My tendency is toward locking devs into specific patterns to obtain consistent behavior from the platform, but this is unrealistic on an open source project for one and a system with such a huge amount of extant modules for two. Generalizing, I imagine a set of automated tests to certify that a module has properly implemented a single policy or several. My goals here are specifying behavior, sandboxing modules, and maintaining system related database tables. Would such an idea generalize extensively beyond uninstallation?

I've got two counter-examples and an example. I've been testing against my patch over in #1268974: For disabled node types with 'base' != 'node_content', _node_types_build() does not return data about the type., though, so I'm curious to see the outcome of testing over here.

oops

Well I got it. The tests should pass, since they don't cover uninstallation (I've been testing against my patch over yonder: #1268974: For disabled node types with 'base' != 'node_content', _node_types_build() does not return data about the type., which should pass the uninstall case as well). I'd still like to add a few docblocks, revise a few docblocks, and add some tests to cover the uninstall case before trying to get this committed. In particular, I'd like to exercise the hook_node_type_delete() call with missing data, but the test will fail. So I guess I write the tests for both Node and Node Example. But how does one get a test added that fails against the current build?

I think we're miscommunicating here. I mean that hook_node_type_delete() should have access to some data, but it doesn't because of a bug. I wrote that post late, though. I'm torn between adding the tests now, effectively blocking this patch until that bug is fixed, or leaving it as is for now. I think that this patch reflects how the system is intended to be used (my first content type, I followed the example but used a base!=node_content, as the documentation claims is okay, resulting in errors in node_type_delete() calls--a sour first experience).

Over in #1268974: For disabled node types with 'base' != 'node_content', _node_types_build() does not return data about the type., though, it seems more people are paying attention than I thought, so maybe this is a nonissue. I think the patch above will be ready after adding some more code comments.

The content types summary page gets generated in /modules/node/content_types.inc and relies on the existence of hook_form() to work properly. The code is ugly and adds a requirement for base!=node_content implementors to explicitly implement a $base_form() that uglifies the example and such content types in general. After a _node_types_build() fix is commited, I'll probably work to remove the requirement and then that bit of code from the example.

It's only necessary for base!=node_content. In content_types.inc (I think) the hook is used to populate the Content Types administrative form, but for base==node_content, Node module enabled => node_content_form() available.

But the `missing' data is sitting in the database with its disabled flag set. The current _node_types_build() qualifies its query to exclude the missing data on rebuilds. This is low hanging fruit, as the data is in place. The reservation against fixing it is that callers may have come to rely on the data set excluding disabled types. A caller within the module was depending on this, so I simply used php's array_filter function to remove disabled elements (after including disabled elements). Tests pass, but I worry about breaking D6->D7 migration and contrib modules. The existing @return says that the disabled types are included without reservation, though, so callers should have been filtering the data.

As to the broader CRUD issues, I doubt many worry about D. If I wasn't just starting, I probably wouldn't care myself. I'm just annoyed that one would bother creating the module installation user interface, with dependency tracking and enforced dependencies, without extending to the entire system. I saw merlinofchaos's blog post discussing the migration from the old module manager to the new one, so I imagine the whole framework used to look similar. There's just not much value in D, even the module tracking system is more about CR. With D one could feel satisfied in having built an elegant system, but at the end of the day I doubt many Drupal sites are actively worked on after their initial construction.