Problem/Motivation

Information about custom node types defined by modules that don't use the default "base" of "node_content" cannot be retrieved after the module is disabled.

Steps to reproduce expected result:

  1. Install Drupal 8.
  2. Enable the book module.
  3. Disable the book module.
  4. Verify that drush eval "print_r(node_type_get_type('book'));" returns an object.

Steps to reproduce a failing result:

  1. Write a small module that just implements hook_node_info() with a base that is not "node_content" (example implementation below).
  2. Enable your module.
  3. DIsable your module.
  4. Verify that drush eval "print_r(node_type_get_type('your_custom_type_name'));" does not return an object.
/**
 * Implements hook_node_info().
 */
function node_sample_node_info() {
  return array(
    'node_sample' => array(
      'name' => 'Our custom node type',
      'base' => 'not_node_content',
      'description' => 'This is a description',
    ),
  );
}

Proposed resolution

Fix _node_types_build() so that it returns information about all disabled node types. Possibly fix the API documentation for _node_build_types() to make it unambiguous that both enabled and disabled types are returned.

Remaining tasks

Needs review of automated tests and re-roll of D7 patch to D8. Eventual backport of the accepted patch to D7.

User interface changes

None.

API changes

None.

Original report by tpopham

Currently I have a node type implemented by hook_node_info() (which has not been deprecated, right?), and I get warnings from the comment uninstall hook at uninstall. If I had attached comments (I may, yet), the warnings would map to litter in my field_config_instance table.

The _node_types_build() documentation technically contradicts itself, with one of the specs being fulfilled, but I've noticed a few other functions that depend on the behavior of the unfulfilled spec. I myself consider this a code bug, but I expect many to jump to 'fix the documentation.' I traced this behavior from a broken node_type_delete() call, see the discussion following the closure of this bug.

The brief description of _node_types_build() claims that the function returns a list of available node types; this is the provided functionality. The return description, however, claims that the function returns a more robust data structure, including whether a particular list constituent is disabled. The functions node_type_get_types() and node_type_get_names() depend on the former behavior, but node_type_get_name(...) depends on the latter. While some may claim that node_type_get_name() behaves as spec'ed, the signature, by accepting a node type string or a node contradicts such a claim, as there may exist nodes that have their types disabled.

The robust returned data structure seems designed to facilitate this work flow (and others I'm sure), but its surrounding code seems to misuse it. I'm going to start on a patch that simply implements things as they seem spec'ed, and see how many broken tests result. Since the method is technically private (I'm new to these parts, but I assume that the leading underscore indicates internal use only), I hope it works out.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcingy’s picture

Priority: Critical » Major

This does not make drupal usable.....

tpopham’s picture

FileSize
11.79 KB

Well I have my first patch. Poking around, I get the impression that node_content has become a sentinel value to obtain node types that still permit node creation after their module has been disabled and possibly even removed. This behavior seems sensible for UI created content since it is built and deleted through the interface, but for a module I can't imagine that being considered best practice.

Unfortunately all of the tests and examples related to node construction now all use base==node_content (go ahead and grep around if you don't believe me). I get the feeling that the code has been hacked to work around disfunctional, vaguely spec'ed behavior from the node module. I'm afraid that the current implementation is gaining inertia as more sites implement toward it--as long as everyone implements with the same sentinel, though, I guess that leaves freedom to refine the spec. Noncritical indeed.

tpopham’s picture

Status: Active » Needs review
FileSize
11.79 KB

Try that again

Status: Needs review » Needs work

The last submitted patch, nt_hook_ok.patch, failed testing.

tpopham’s picture

Status: Needs work » Needs review
FileSize
20.66 KB

Giving it another go. hook_node_info() and node_type_save() created node types have their disabled states anticoupled to the module's enabled state, unless base==node_content. I think having the the rebuild case having the extra database filter was causing the junk comment errors.

Status: Needs review » Needs work

The last submitted patch, nt_hook_ok2.patch, failed testing.

tpopham’s picture

Status: Needs work » Needs review
FileSize
31.04 KB

Well I figured out the testing environment so that I can stop littering these logs with my laziness. I got me something that seems to work well. I've rebuilt the _node_types_save() body, and I also updated some antiquated documentation. For the record, I think the buggy behavior that led me to this was caused by the non-disabled predicate put on the database query during rebuilds. The new version may not perform as well, but I anticipate the cached version being used outside of administration tasks, so I don't think that that is an issue (if in fact it doesn't perform as well--benchmark away if somebody cares to).

tpopham’s picture

Status: Needs review » Needs work

My one reservation is behavior when a dev alters their hook_node_info(). I feel that any changes in the node_type table should be preserved. If a altered hook_node_info() is implemented, then the dev should either implement an explicit update to migrate the changes into the database, with installers using the new hook_node_info() to set the node_type table row. No one expects a changed hook_node_info() to propagate, right? There's a code comment where I avoided possibly including altered naming data from hook_node_info(). I shouldn't be saving changed fields in addition to the name back to the database, should I? I wouldn't mention, but I see such a workflow slipping by testing.

tpopham’s picture

Status: Needs work » Needs review

Oops. I'm still new to this

tpopham’s picture

Title: The _node_types_build() Function Behaves Inconsistent with its Documentation » For node types with base!=node_content, hook_node_type_delete() parameter data is missing

Removing a node type with its type disabled results in:
Notice: Trying to get property of non-object in comment_node_type_delete()
This commonly occurs for modules implementing a node type and that bother to call node_type_delete() during their uninstall (see Node Example for an example, although the author fortunately chose base==node_content, ensuring that the type is never disabled and therefore never triggers this bug). Implemented with base!=node_content, however, a module must be disabled before uninstalled, thus triggering the bug.

I anticipate similar errors from any hook_node_type_delete() implementors that are enabled under the aforementioned conditions.

This comment belongs up top (sorry). I just realized that search traffic would never find this page as...was.

Mile23’s picture

subscribe

tpopham’s picture

The `add content' menu includes a type defined by the blog module while it is disabled. The mistaken item survives a manual cache reset. The database does properly reflect the module state with disabled=1. There existed some weird logic in the old implementation that predicated the database query on disabled=0. I'm betting that this is responsible for the new bug. Tracing through system_admin_menu_block().

tpopham’s picture

FileSize
32.06 KB

The node_menu() hook clears the cache, so the non-rebuild call from node_type_get_types() to _node_type_build() doesn't grab from cache, thus managing to trigger that qualified database query and doesn't return disabled node types. Forgivable if the cache clearing function were private to the module, but a definate flaw. At this point I'm still hopeful about getting the patch applied, as I don't see external API's calling for cache resets, and if contrib modules don't use it to obtain this behavior, then the patch may clear wider testing.

Since the node_menu() hook doesn't alter the node type data before using enabled types to generate the menu items, the cache reset is unnecessary and has been removed (I suspect it was a work around used to obtain the menu without including the disabled types).

tpopham’s picture

I just noticed the now disabled blog entry node type when constructing a node type driven context. This seems inappropriate in my case, as there exists no Blog entry nodes. In the typical case, though, where many nodes may have been constructed before disabling the module (and therefore its type), this seems proper.

tpopham’s picture

Reworking the Node Example module, I noticed an old bug where after install a new node type is disabled. I tracked the bug to the logic where the enabled state of a node type is coupled to the enabled state of its module. For base!=node_content with the node type created by a direct call to node_type_save, the module column in the {node_type} table is empty. So testing if the $db_type->module for membership amongst currently active modules fails.

tpopham’s picture

Looking in node_type_set_defaults(), I notice no default value for 'module'. If the field is deprecated in lieu of the 'base' field, which is my impression from some D6->D7 notes that I saw once upon a time, then this could be a problem. The logic that converts hook_node_info() data to node types should contain the solution, and as long as hook_node_info() is supported, then the solution should continue working.

tpopham’s picture

Never mind.

  if (empty($new_type->module)) {
    $new_type->module = $new_type->base == 'node_content' ? 'node' : '';
  }

Node types implemented with direct calls to node_type_save() cannot propagate the implementing module into the database. Forget base==node_content: any type that was not built by hook_node_info() will not disable with its constructing module, because the data is not available. This has to be a flaw, or else why bother with the node_content sentinel? As a workaround, a user could provide 'module'=>'mymod' to a direct call of node_type_save(), and the information would find its way into the database, but the D6->D7 doc that I mentioned earlier deprecated the 'module' key. The obvious solution in my mind is to specify that module node types must be implemented with hook_node_info(). While this requirement is too strong if the module developer plans to use base==node_content, at least things would behave as they ought to.

As a side note, leaving base=='' could just as easily have implemented the current behavior, but then I'm not too worried about reverse compatibility. If one could leap to the conclusion that base and module are the same, then all would be fine, but my impression is that 'base' replaced 'module' so that hook_insert(), et al., could be unique to each node type within a module.

tpopham’s picture

FileSize
34.66 KB

Rereading the hook_node_info() docs:

Only module-provided node types should be defined through this hook. User- provided (or 'custom') node types should be defined only in the 'node_type' database table, and should be maintained by using the node_type_save() and node_type_delete() functions.

I think the original intention seems clear in the light from above. The second sentence seems strangely useless, but the first sentence needs the `Only' excluded, right? And maybe `should' |--> `must'?

I'm going to treat orphaned nodes (those without a corresponding module field) as equivalent to base=node_content, as that seems the old (and therefore expected) behavior. I think warnings are warranted for

  • base!=node_content && undefined module
  • base==node_content && defined module

The former case will be tripped by modules that explicitly do not want orphaned behavior, as indicated by the choice of base!=node_content, but will obtain that behavior anyway because they were not implemented by a hook_node_info(). The Blog module, presumably amongst others, will trip the latter case, but this is an oversight of its authors. A disabled module should prevent construction of its types; disabled types are still available to contexts, etc. that don't filter (or by design choose to filter) the results of node_type_get_types(), etc.

tpopham’s picture

Apologies to the Blog authors. The blog module implements its node type by hook_node_info() with base==blog, obtaining reasonable behavior.

catch’s picture

@tpopham - could you roll your patch as a single git diff origin/7.x or git diff origin/8.x? It's hard for human reviewers to evaluate changes when the patches are all stacked on top of each other even if git itself is happy to apply them.

Some of the history of the disabled field can be found in #895014: All fields of a node type are lost on module disable - this was a last minute Drupal 7 critical bug so it doesn't surprise me at all that there's some follow-up to do.

Haven't read through the whole issue yet.

tpopham’s picture

FileSize
13.7 KB

Of course. I had to poke a few different behaviors with a stick before I could figure out exactly how this was supposed to behave, and some of the intermediate diffs reflect that learning process. I thought others might want to see that process as well. Out of curiosity, when I see in other bug reports someone submitting a patch that 'rerolls' the current patch, are they typically removing intermediate changes that everyone working on the patch seem to agree on, or are there other reasons to reroll a patch?

I'm not precisely sure of the error handling policies in Drupal. I used drupal_set_message() to log errors, and placed them where they would likely annoy only during installation. Does this sort of thing belong to watchdog or some other error logger? I'd rather have the error generated at construction and not have to worry about annoying.

tpopham’s picture

I'd like to see node types automatically removed at uninstall. Toward the tail of #895014-9: All fields of a node type are lost on module disable I see this functionality was probably excluded to keep System simple and facilitate upgrades. Is the hook system sufficiently robust for uninstallation to trigger something on a module's node entities? Such a hook seem the natural place to implement this functionality without extending tentacles into other modules. As a side note, I hate that disabled flag (maybe this started as a workflow that was lazily integrated into the schema or something, but it has probably propagated into contrib modules sufficiently to discourage fixing it--now or in D10).

I can't remember where I mentioned it, but I like the idea of defining policies (and tests to verify the policies) to certify modules against. In particular, a module could implement a `remove all data and metadata' policy and a `remove only metadata' policy, and the update UI could query users for which case to use at uninstall. This could definitely remove some of the risk in incorporating new modules and ensure smoother upgrades between major versions.

I remember explicitly commenting the handling of #895014-10: All fields of a node type are lost on module disable. UI customization doesn't get clobbered when reenabling a module (at least it shouldn't--I need to dig around in the tests a little better and maybe include a couple more).

I actually saw #895014: All fields of a node type are lost on module disable amongst others before I started this issue. I tried to reference the few of them that were open still. I think that the committed patch was hacked many times and that the implementation stabilized around the flawed uninstall (I mean the calls to hook_node_type_delete() without data, by the way, not auto-removing node types at uninstall), because tests were/are lacking in that area. I guess I should be adding a test to explicitly exercise the bug, also. By the way, I've been cleaning up the Node Example over in #1268112: Disabled Node Example module still permits construction of Node Example content, and I'd like to duplicate such a test over there as well.

Mile23’s picture

Version: 7.8 » 8.x-dev
Issue tags: +Needs backport to D7

Putting this where eyes can see it....

catch’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs issue summary update

While #1181030: Node types disabled by _node_types_build() when using node_type_save() with base != 'node_content' was the older issue, this has more activity so I've marked it as duplicate of this one.

This needs automated tests to show the bug. Also adding issue summary tag.

Mile23’s picture

Google isn't my friend on this... Is there a way to uninstall a module in the test regime?

spydmobile’s picture

Has anyone seen this?
http://andypangus.com/an-annoying-error-part-two
looks like it might be a potential D7 patch?
Franco

bdurbin’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs issue summary update
FileSize
1.44 KB

Adding a test to exercise what seems to be core of the issue: information about disabled custom node types with a "base" that is not "node_content" is not returned in node_type_get_type()/_node_types_build().

It's worth noting that there are other consumers of _node_types_build() that might suffer similar problems. As novice core test writers, we're looking for feedback on the test itself and whether or not we need to expand on what we have.

Props to: jjhnbrnn and magnusk for their collaboration on this patch at the DrupalCon Denver Core Office Hours Sprint.

jhnbrnn’s picture

Status: Needs work » Needs review
FileSize
13.46 KB

Re-rolled the patch against 8.x.

magnusk’s picture

We've updated the issue summary to match the standard.

magnusk’s picture

Issue tags: -Needs tests

We've checked that the test passes after re-rolling the patch.

bdurbin’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -Needs issue summary update
FileSize
1.44 KB

Re-rolled the test (syntax error [blerg]).

bdurbin’s picture

FileSize
1.44 KB

Re-submitting tests.

Status: Needs review » Needs work

The last submitted patch, 1268974_tests_32.patch, failed testing.

bdurbin’s picture

Title: For node types with base!=node_content, hook_node_type_delete() parameter data is missing » For disabled node types with 'base' != 'node_content', _node_types_build() does not return data about the type.
Status: Needs work » Needs review

Our new test is the one failing in #32 (expected pre-patch behavior), so this ticket now needs review of the new tests in #32 and the re-rolled D8 patch in #28.

magnusk’s picture

Should the function patch in #28 and the test patch in #32 be merged into one patch file? this way the System Message wouldn't change the status to "needs work" after running the tests (it wouldn't fail) and I'm guessing a single patch file would be required for someone to commit "the patch" once reviewed and approved.

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Novice

Thanks @bdurbin, @jhnbrnn, and @magnusk! The summary is definitely clearer now, and it looks like the test makes reasonable assertions and fails as expected when the bug is present.

Regarding this:

Should the function patch in #28 and the test patch in #32 be merged into one patch file? this way the System Message wouldn't change the status to "needs work" after running the tests (it wouldn't fail) and I'm guessing a single patch file would be required for someone to commit "the patch" once reviewed and approved.

The normal method is to upload a test-only patch as the first attachment to the comment, and the combined test+fix patch as the second. This exposes the failures of the test for reviewers without the fix (demonstrating that it does in fact test for the bug) and demonstrates that the patch does in fact fix it without regressions.

Testbot only changes the status of an issue based on the last attachment, so the issue will remain at "Needs Review" if the combined patch is second and does not have any other bugs. Here's an example: http://drupal.org/node/312458#comment-5718088

@bdurbin has already uploaded a test-only patch above in #32, exposing that the assertion for Return data from node_type_get_type is valid for a disabled content type with a base that is not node_content fails without the fix. So the next thing we need is a combined patch that includes both the fix and the test, which should pass. When rolling a combined patch, here's a couple minor things to clean up from #32

  1. +++ b/core/modules/node/node.testundefined
    @@ -1447,6 +1447,32 @@ class NodeTypeTestCase extends NodeWebTestCase {
    +   * Tests handling of node type types that are.
    

    Looks like this is missing the end of the sentence? :)

  2. All the blank lines in the patch have trailing whitespace on them. This should be removed. (I recommend configuring your editor to show trailing whitespace.)
  3. Finally, a couple brief inline comments in the test explaining/summarizing what's being done in the test would be helpful.

I also reviewed the original patch, and found a number of style issues with it, so it would be good to clean those up as well when creating the combined patch. In general, the patch comments need a lot of work. They should be complete, clear sentences that begin with a capital and end with a period. I'd also suggest rewriting a lot of them to be more clear and concise. (This would be best done by someone with good English skills.) A number of specific points for that patch follow:

  1. +++ b/core/modules/node/node.moduleundefined
    @@ -497,7 +498,8 @@ function node_type_load($name) {
    + * Saves a node type to the database. Modules should not implement node types
    + * with this function, but should use hook_node_info() instead.
    

    The second sentence should be in a separate paragraph after a blank line. Reference: http://drupal.org/node/1354#functions

  2. +++ b/core/modules/node/node.moduleundefined
    @@ -509,6 +511,18 @@ function node_type_save($info) {
    +  } elseif (!$type->custom && $type->module == '') {
    
    @@ -707,65 +730,109 @@ function _node_types_build($rebuild = FALSE) {
    +    } else {
    ...
    +      if ($type->module=='' || (isset($type->base) && $type->base=='node_content')) {
    ...
    +      } elseif (isset($enabled_modules[$db_type->module])) {
    ...
    +      } else {
    

    This formatting for if/else logic does not follow our coding standards; the curlies should be on their own lines. Reference: http://drupal.org/coding-standards#controlstruct

  3. +++ b/core/modules/node/node.moduleundefined
    @@ -669,30 +683,39 @@ function node_type_update_nodes($old_type, $type) {
    +  * Get the list of all node types that have not been deleted by a call to
    +  * node_type_delete().
    

    The first word here should be a third-person verb (e.g. "Gets"), and the comment should also be reworded to be one line of fewer than 80 characters. Reference: http://drupal.org/node/1354#functions

  4. +++ b/core/modules/node/node.moduleundefined
    @@ -669,30 +683,39 @@ function node_type_update_nodes($old_type, $type) {
    +  * This function provides the `node_type' table state as it would appear after a
    

    (And subsequent) It's more normal to use curlies to denote a table name rather than backticks. (Backticks are mysql-specific, but curlies are used in Drupal's query builder.)

    Also, this docblock needs to be rewrapped a little; many lines are a bit over 80 characters.

  5. +++ b/core/modules/node/node.moduleundefined
    @@ -669,30 +683,39 @@ function node_type_update_nodes($old_type, $type) {
    +  * type's `disabled' property indicates if the node type's implementing module
    

    The word 'disabled' should be in single quotes, if anything (no opening backtick).

  6. +++ b/core/modules/node/node.moduleundefined
    @@ -669,30 +683,39 @@ function node_type_update_nodes($old_type, $type) {
    +  * `node_content' always remains non-disabled (making Node Content a
    +  * poor name choice for a module).
    

    I don't think we need the editorial here in parentheses.

  7. +++ b/core/modules/node/node.moduleundefined
    @@ -707,65 +730,109 @@ function _node_types_build($rebuild = FALSE) {
    +      // active_type not in node_type table => active_type is new, so clobber
    +      // this default value if the type is found in the database.
    ...
    +    // Clobber any type information from hook_node_info() with that from the
    +    // database to preserve any modifications
    

    Let's not use the word "clobber."

  8. +++ b/core/modules/node/node.moduleundefined
    @@ -707,65 +730,109 @@ function _node_types_build($rebuild = FALSE) {
    +  // Gather node types from the node_type table (and I have no clue what the
    +  // translatable tag does)
    

    Let's remove the parenthetical about the translatable tag. :)

  9. +++ b/core/modules/node/node.moduleundefined
    @@ -707,65 +730,109 @@ function _node_types_build($rebuild = FALSE) {
    +  // Could condition on base!=node_content and/or custom=0, but then a second
    +  // query and loop would be necessary to add these results verbatim to the
    +  // data structure
    

    I think we can just remove this comment.

  10. +++ b/core/modules/node/node.moduleundefined
    @@ -707,65 +730,109 @@ function _node_types_build($rebuild = FALSE) {
    +  // Reconcile node_type table against the enabled modules list. Any node type
    +  // with base!=node_content inherits its disabled state from that of its ¶
    +  // module.
    

    This comment should have an article ("the node type table") and a different word than "reconcile" would also be better. Let's say "with a base other than node_content" rather than the pseudocode. Also, there's a bit of trailing whitespace after "its".

  11. +++ b/core/modules/node/node.moduleundefined
    @@ -707,65 +730,109 @@ function _node_types_build($rebuild = FALSE) {
    +      // A hook_node_info() was unavailable for the current iterant from the
    +      // database, so enable or disable the iterant depending on its module's
    +      // state
    

    The word "iterant" is not commonly used; I'd recommend rewording this comment to be a bit simpler and clearer.

  12. +++ b/core/modules/node/node.moduleundefined
    @@ -707,65 +730,109 @@ function _node_types_build($rebuild = FALSE) {
    +    // This was inside the hook_node_info()-unavailable-case above, since the
    +    // name data from the hook_node_info() call should be fine to return, but
    +    // changes in hook_node_info() may not have propagated into the database yet
    +    // (and a rebuild won't fix a changed name, so the spec comes into play:
    +    // return what the node type table would look like after a rebuild).
    

    We don't need to reference the pre-patch behavior here; instead, it would be better to just explain the what/why more briefly.

  13. +++ b/core/modules/node/node.moduleundefined
    @@ -707,65 +730,109 @@ function _node_types_build($rebuild = FALSE) {
    +        // explicitly remove the new_state flag or someone will start depending
    +        // on unspec'ed behavior
    
    @@ -3723,7 +3789,8 @@ function _node_access_rebuild_batch_finished($success, $results, $operations) {
    +  // It is impossible to define a content type with base!=node_content without
    +  // also implementing hook_form()
    

    I'd recommend expanding these comments to not use abbreviations ("unspecified" rather than "unspec'ed" and "a base that is not node_content" rather than the pseudocode there).

  14. +++ b/core/modules/node/node.moduleundefined
    @@ -2086,10 +2153,9 @@ function node_menu() {
    +  $enabled_types = array_filter(node_type_get_types(),
    +                                create_function('$t','return !$t->disabled;'));
    

    The indentation here is weird. It should either be all on one line, or with each argument on its own line and indented by two spaces.

    We also don't use anonymous functions like this elsewhere in core that I've seen, so I'm on the fence as to whether it would be better to rewrite this differently. At the least, we should have an inline comment explaining this line.

It would be a good novice task here to create a combined patch that addresses the points outlined above. Thanks everyone!

ryan.gibson’s picture

Assigned: Unassigned » ryan.gibson

I'll take this one. :)

ryan.gibson’s picture

Here's the combined patch - I followed along with @xjm and the comments she made. Also, @timplunkett helped, especially with regards to comment #14 from @xjm.

I didn't make the inline comments from comment #3 from @xjm. Wasn't quite sure what was going on well enough to do that.

Let me know if there is something to be fixed and I'll get on it! Also, I can make those inline comments, I just would need to be walked through what is going on in the test.

ryan.gibson’s picture

Status: Needs work » Needs review

Oops, changing to NR.

tim.plunkett’s picture

Status: Needs review » Needs work
+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+      // Copy subset of the types property's data to the name property

Missing trailing full stop. Also, I think this comment is wrong. The code is extracting the name from the type info, and copying it into the names array.

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+  // Gather node types from the node_type table

Missing trailing full stop.

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+    // database to preserve any modifications

Missing trailing full stop.

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+      else {

Indented 2 spaces too many

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+      // state

Missing trailing full stop.

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+      if ($type->module=='' || (isset($type->base) && $type->base=='node_content')) {

Needs spaces on either side of ==

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+        // Special case: always enabled

Missing trailing full stop.

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+        elseif (isset($enabled_modules[$db_type->module])) {

Indented 2 spaces too many

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+        // The module is enabled

Missing trailing full stop.

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+        else {

Indented 2 spaces too many

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+        // The module is disabled, uninstalled, or missing entirely

Missing trailing full stop.

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+    // Don't litter the return data structure with internal data

Missing trailing full stop.

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+        // explicitly remove the new_state flag or someone will start depending

Should start with a capital letter.

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+        // on unspecified behavior

Missing trailing full stop.

+++ b/core/modules/node/node.moduleundefined
@@ -3719,7 +3780,8 @@ function _node_access_rebuild_batch_finished($success, $results, $operations) {
+  // node_content without also implementing hook_form()

Missing trailing full stop.

+++ b/core/modules/node/node.testundefined
@@ -1447,6 +1447,32 @@ class NodeTypeTestCase extends NodeWebTestCase {
+   * Tests handling of node type types that are undefiined.

Typo: undefined

ryan.gibson’s picture

Status: Needs work » Needs review
FileSize
14.93 KB

Made changes after the comments by @timplunkett.

tim.plunkett’s picture

Status: Needs review » Needs work
+++ b/core/modules/node/node.moduleundefined
@@ -388,9 +388,12 @@ function _node_extract_type($node) {
+ * explicitly removed by node_type_delete or direct database interaction will

node_type_delete should have ()

+++ b/core/modules/node/node.moduleundefined
@@ -388,9 +388,12 @@ function _node_extract_type($node) {
  * See _node_types_build() for details.

I think this should be @see _node_types_build()

+++ b/core/modules/node/node.moduleundefined
@@ -499,6 +500,9 @@ function node_type_load($name) {
+ * Modules should not implement node types
+ * with this function, but should use hook_node_info() instead.

This should be rewrapped to be as long as possible without going over 80 chars.

+++ b/core/modules/node/node.moduleundefined
@@ -509,6 +513,19 @@ function node_type_save($info) {
+    // A modular node type with base==node_content does not behave like something

I think the the == should be replaced with English, and is that the right usage of "modular"?

+++ b/core/modules/node/node.moduleundefined
@@ -509,6 +513,19 @@ function node_type_save($info) {
+    // content_types.inc uses node_hook() to build the `Content types' list,

Shouldn't use a backtick here, and I think this should be reworded to start the sentence with a capital.

+++ b/core/modules/node/node.moduleundefined
@@ -509,6 +513,19 @@ function node_type_save($info) {
+    drupal_set_message(t('The modular content type %type must either use base==node_content or be implemented by hook_node_info().', $map), 'error');

See two comments above, about modular and ==.

+++ b/core/modules/node/node.moduleundefined
@@ -669,30 +686,38 @@ function node_type_update_nodes($old_type, $type) {
+  * Gets a list of all node types that aren't  deleted by node_type_delete().

Two spaces in here. Should "aren't" be "were not"? Or can this be reworded to not refer directly to a function?

+++ b/core/modules/node/node.moduleundefined
@@ -669,30 +686,38 @@ function node_type_update_nodes($old_type, $type) {
+  * a rebuild, actually rebuilding the table for $rebuild==TRUE. The returned

spaces around ==

+++ b/core/modules/node/node.moduleundefined
@@ -669,30 +686,38 @@ function node_type_update_nodes($old_type, $type) {
+  * module has been disabled, however, any node type with a {base} value of
+  * {node_content} always remains non-disabled making Node Content a

The curlies generally refer to a DB table, as in node_type at the beginning of the paragraph.

+++ b/core/modules/node/node.moduleundefined
@@ -669,30 +686,38 @@ function node_type_update_nodes($old_type, $type) {
+  * poor name choice for a module.

"poor" seems not assertive enough.

+++ b/core/modules/node/node.moduleundefined
@@ -669,30 +686,38 @@ function node_type_update_nodes($old_type, $type) {
+  *  TRUE to rebuild {node_types} table of the database.

This needs to be indented one space further.

+++ b/core/modules/node/node.moduleundefined
@@ -669,30 +686,38 @@ function node_type_update_nodes($old_type, $type) {
+  *     $rebuild==TRUE. The property is a thin version of the {types} property.

Rewrite to use English, not ==.

+++ b/core/modules/node/node.moduleundefined
@@ -669,30 +686,38 @@ function node_type_update_nodes($old_type, $type) {
+  *     - disabled: If {base==node_content}, then this property does not vary

Rewrite to use English, not ==.

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+      // active_type not in node_type table => active_type is new, so remove
+      // this default value if the type is found in the database.

This should be reworded to start with a capital, and should be less pseudo-code and more English.

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+      $active_types->types[$type]->is_new = 1;

Should use TRUE, not 1

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+      // active_type is new => active_type is not disabled, so be consistent
+      // with other defaults for now

This should be reworded to start with a capital, and should be less pseudo-code and more English. Also, missing trailing full stop.

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+      $active_types->types[$type]->disabled = 0;

Should use FALSE, not 0

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+      // attach flag to indicate whether the type needs to be (re)written to the
+      // database

This should be reworded to start with a capital, and should be less pseudo-code and more English. Also, missing trailing full stop.

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+         $active_types->types[$type]->new_state = 1;

Indented one space too many

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+  // Gather node types from the node_type table.

Should have {} around the table name.

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+  // Sync the node_type table against the enabled modules list. Any node type
+  // with a base other than node_content inherits its disabled state from that

Should have {} around the table name.

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+    // Remove any type information from hook_node_info() with that from the

"with that from the database" doesn't sound right.

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+      // database, so enable or disable the occurance depending on its module's
+      // state

Missing trailing full stop.

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+    if ($type->module == '' || (isset($type->base) && $type->base == 'node_content')) {

Now these are outdented too far.

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+        // Special case: always enabled

Missing trailing full stop

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+        $new_state = 0;
+        $type->disabled = 0;

Use FALSE, not 0

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+    elseif (isset($enabled_modules[$db_type->module])) {

Now these are outdented too far.

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+        $new_state = ($type->disabled != 0);
+        $type->disabled = 0;

Use FALSE, not 0

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+    else {

Now these are outdented too far.

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +732,101 @@ function _node_types_build($rebuild = FALSE) {
+        $new_state = ($type->disabled != 1);
+        $type->disabled = 1;

Use FALSE, not 0

+++ b/core/modules/node/node.moduleundefined
@@ -1256,8 +1317,8 @@ function node_delete_multiple($nids) {
+        // because node module is implementing search module's API,
+        // not the other way around.

This looks like it's wrapped too early.

+++ b/core/modules/node/node.testundefined
@@ -521,7 +521,7 @@ class NodeCreationTestCase extends NodeWebTestCase {
-   * Create a page node and verify that a transaction rolls back the failed creation
+   * Create a page node and verify that a transaction rolls back the failed creation.

This should be rewritten to be under 80 chars.

+++ b/core/modules/node/node.testundefined
@@ -1447,6 +1447,32 @@ class NodeTypeTestCase extends NodeWebTestCase {
+   * Tests handling of node type types that are undefinned.

undefined is now spelled wrongly a different way.

+++ b/core/modules/node/node.testundefined
@@ -1447,6 +1447,32 @@ class NodeTypeTestCase extends NodeWebTestCase {
+      ->fields(array('disabled' => 1))

TRUE, not 1

+++ b/core/modules/node/node.testundefined
@@ -1447,6 +1447,32 @@ class NodeTypeTestCase extends NodeWebTestCase {
+    $this->assertTrue(($updated == 1), 'Successfully disabled custom content type.');

TRUE, not 1

ryan.gibson’s picture

Here it is again. A big thanks to @timplunkett for his mentorship and to many others for continuing to help me along! I should also say thanks in advance to @timplunkett as I'm sure he'll tear this one apart as well and I'll learn a ton of new things. ;)

tim.plunkett’s picture

Last pass, I think.

+++ b/core/modules/node/node.moduleundefined
@@ -388,10 +388,13 @@ function _node_extract_type($node) {
+ * @see _node_types_build() for details.

This should be below the @return, with a blank line before it, and just @see _node_types_build(), no punctuation or strings after it.

+++ b/core/modules/node/node.moduleundefined
@@ -509,6 +513,19 @@ function node_type_save($info) {
+    // The {Content types} list is built by content_types.inc using node_hook().

'{Content types}' looks wrong to me. Probably just 'content types'

+++ b/core/modules/node/node.moduleundefined
@@ -669,30 +686,39 @@ function node_type_update_nodes($old_type, $type) {
+  * a rebuild, actually rebuilding the table for $rebuild == TRUE. The returned

Missed one instance of rewriting the == to English.

+++ b/core/modules/node/node.moduleundefined
@@ -669,30 +686,39 @@ function node_type_update_nodes($old_type, $type) {
+  * node_content always remains non-disabled making Node Content a

node_content should likely be in single quotes, and put a comma after non-disabled

+++ b/core/modules/node/node.moduleundefined
@@ -669,30 +686,39 @@ function node_type_update_nodes($old_type, $type) {
+  *   TRUE to rebuild {node_types} table of the database.

{node_type} is singluar

+++ b/core/modules/node/node.moduleundefined
@@ -669,30 +686,39 @@ function node_type_update_nodes($old_type, $type) {
+  *     $rebuild is equal to true. The property is a thin version of the {types}

{types} should be wrapped in single quotes, not curly braces

+++ b/core/modules/node/node.moduleundefined
@@ -669,30 +686,39 @@ function node_type_update_nodes($old_type, $type) {
+  *     Like the {names} array, this data structure does not necessarily reflect

Same, replace { with '

+++ b/core/modules/node/node.moduleundefined
@@ -669,30 +686,39 @@ function node_type_update_nodes($old_type, $type) {
+  *       property does not vary from 0. Otherwise, the $type->disabled flag
+  *       takes a value of 0 for any type in the {node_type} table whose module
+  *       is enabled, or it takes a value of 1 if its module is disabled
+  *       (or possibly uninstalled).
+  *     - is_new: The $type->is_new flag does not appear in the {node_type}
+  *       table. The property gets set to 1 for node types implemented by
+  *       hook_node_info() that did not appear in the {node_type} table at
+  *       _node_types_build() entry. The property is left unset otherwise.

I guess the 0s and 1s here should be FALSE and TRUE. Sorry.

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +733,99 @@ function _node_types_build($rebuild = FALSE) {
+      // Overwite the defaults of is_new and disabled for the new active type.

Overwrite is spelled wrong.

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +733,99 @@ function _node_types_build($rebuild = FALSE) {
+  // with a base other than node_content inherits its disabled state from that

Wrap node_content in single quotes.

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +733,99 @@ function _node_types_build($rebuild = FALSE) {
+      // A hook_node_info() was unavailable for the current occurance from the
+      // database, so enable or disable the occurance depending on its module's

occurrence is spelled wrong twice

ryan.gibson’s picture

And made those changes.

magnusk’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1268974-cleanup_and_fix_errors-45.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review

Go, bot, go!

xjm’s picture

Whoops. I think there may be an issue with QA. Retesting the patch.

Status: Needs review » Needs work

The last submitted patch, 1268974-cleanup_and_fix_errors-45.patch, failed testing.

xjm’s picture

Alright, it was a coincidence that I saw this error twice from the same bot on two separate patches. The patch does in fact introduce a syntax error. I confirmed this locally:

[kepler:drupal | Thu 16:11:22]$ git apply 1268974-cleanup_and_fix_errors-45.patch 
[kepler:drupal | Thu 16:11:28]$ php -l core/modules/node/node.module 
Parse error: parse error in core/modules/node/node.module on line 527
xjm’s picture

This is line 527 with the patch applied:

    drupal_set_message(t('"The content type %type must either have 'base' key set to 'node_content' or be defined via hook_node_info()."', $map), 'error');

There are single quotes outside double quotes with additional single quotes in the string. That'd be the problem. ;)

ryan.gibson’s picture

Alright, removed the single quotes.

ryan.gibson’s picture

Status: Needs work » Needs review
xjm’s picture

Status: Needs review » Needs work

Awesome work on this so far. The latest revision of this patch is much cleaner and easier to understand than the earlier versions. Unfortunately this means I understand it well enough to spot some more minor issues with the documentation. :)

  1. +++ b/core/modules/node/node.moduleundefined
    @@ -388,14 +388,17 @@ function _node_extract_type($node) {
    + * Returns a list of a site's known node types.
    +
    + * Any node type added by hook_node_info() or node_type_save() that has not been
    

    Missing asterisk here on the blank line.

  2. +++ b/core/modules/node/node.moduleundefined
    @@ -499,6 +500,9 @@ function node_type_load($name) {
    + * Modules should not implement node types with this function, but should use
    + * hook_node_info() instead.
    

    I think we should say either "create node types" or "define node types." "Implement" does not seem correct in this context.

  3. +++ b/core/modules/node/node.moduleundefined
    @@ -509,6 +513,19 @@ function node_type_save($info) {
    +    // A module provided node type whose 'base' key is 'node_content' will not
    +    // be automatically disabled on disabling of it's parent module.
    

    The phrase "module-provided" should be hyphenated, and "whose" personifies the node type unnecessarily. Also, "its parent module." http://theoatmeal.com/comics/misspelling

    Actually... I'd rewrite this as:
    If a module provides a node type with a 'base' key of 'node_content', the node type will not be automatically disabled when the module is disabled.

  4. +++ b/core/modules/node/node.moduleundefined
    @@ -509,6 +513,19 @@ function node_type_save($info) {
    +    drupal_set_message(t('The content type %type will not be disabled with its parent module.', $map), 'warning');
    

    Is it really a "parent" module? Maybe we can just say "The content type %type will not be disabled"?

  5. +++ b/core/modules/node/node.moduleundefined
    @@ -509,6 +513,19 @@ function node_type_save($info) {
    +    // But, when the module is disabled, the [base]_form vanishes if it was
    

    s/But/However. Also, the word "vanishes" is a bit metaphorical; can we phrase this more literally and explain what is actually happening? As opposed to vanishing, which would be way cool, but seems unlikely. :)

  6. +++ b/core/modules/node/node.moduleundefined
    @@ -669,30 +686,39 @@ function node_type_update_nodes($old_type, $type) {
    +  * This function provides the {node_type} table state as it would appear after
    +  * a rebuild, actually rebuilding the table when $rebuild is true. The returned
    

    TRUE should be all in caps. I also think we could make this a little clearer by splitting the sentence into two:
    ...as it would appear after a rebuild. Furthermore, when $rebuild is set to TRUE, the table is actually rebuilt.

  7. +++ b/core/modules/node/node.moduleundefined
    @@ -669,30 +686,39 @@ function node_type_update_nodes($old_type, $type) {
    +  * node type's disabled property indicates if the node type's implementing
    

    Nitpick: "if" should probably be replaced with "whether" here.

  8. +++ b/core/modules/node/node.moduleundefined
    @@ -669,30 +686,39 @@ function node_type_update_nodes($old_type, $type) {
    +  * module has been disabled, however, any node type with a base value of
    

    Verging on a run-on here... let's add a period after "module has been disabled" and make "However" the start of a new sentence:
    ...module has been disabled. However, any node type with a base value of...

  9. +++ b/core/modules/node/node.moduleundefined
    @@ -669,30 +686,39 @@ function node_type_update_nodes($old_type, $type) {
    +  * 'node_content' always remains non-disabled, making Node Content a
    +  * bad name for a module.
    

    Sorry, I think my previous review was unclear about this line. I meant to say that the words "making Node Content a bad name for a module" should be removed, not just that the parentheses should be removed. :)

    Also... I think it would be better to say either:
    ...'node_content' is never disabled...
    or
    ...'node_cotent' always remains enabled...

  10. +++ b/core/modules/node/node.moduleundefined
    @@ -669,30 +686,39 @@ function node_type_update_nodes($old_type, $type) {
    +  *   - names: Associative array of the names corresponding to active or
    ...
    +  *   - types: Associative array of node type objects, keyed by node type.
    

    We should have articles here ("An associative array...").

  11. +++ b/core/modules/node/node.moduleundefined
    @@ -669,30 +686,39 @@ function node_type_update_nodes($old_type, $type) {
    +  *     $rebuild is equal to true. The property is a thin version of the 'types'
    

    TRUE should be in all caps here.

  12. +++ b/core/modules/node/node.moduleundefined
    @@ -669,30 +686,39 @@ function node_type_update_nodes($old_type, $type) {
    +  *       property does not vary from FALSE. Otherwise, the $type->disabled flag
    

    I'd say "is always FALSE" rather than "does not vary from FALSE."

  13. +++ b/core/modules/node/node.moduleundefined
    @@ -669,30 +686,39 @@ function node_type_update_nodes($old_type, $type) {
    +  *       takes a value of 0 for any type in the {node_type} table whose module
    

    Was this another zero that should be replaced with FALSE? (I'm not positive; please double-check.) :)

  14. +++ b/core/modules/node/node.moduleundefined
    @@ -669,30 +686,39 @@ function node_type_update_nodes($old_type, $type) {
    +  *       hook_node_info() that did not appear in the {node_type} table at
    +  *       _node_types_build() entry. The property is left unset otherwise.
    

    I'd say "that do not yet appear in the {node_type} table." (The last bit is confusing, and I think it's implied.)

  15. +++ b/core/modules/node/node.moduleundefined
    @@ -707,65 +733,99 @@ function _node_types_build($rebuild = FALSE) {
    +      // Overwrite the defaults of is_new and disabled for the new active type.
    

    Should we single-quote 'is_new' and 'disabled'?

  16. +++ b/core/modules/node/node.moduleundefined
    @@ -707,65 +733,99 @@ function _node_types_build($rebuild = FALSE) {
    +      // When rebuilding, mark this type as new, to ensure it overwrites any
    +      // existing values in the database.
    

    I think the second comma here is incorrect.

  17. +++ b/core/modules/node/node.moduleundefined
    @@ -707,65 +733,99 @@ function _node_types_build($rebuild = FALSE) {
    +      // Copy data to the names array.
    ...
    +    // Copy data to names array.
    

    UI/API text should use articles ("the names array"). (The first one has the article but the second doesn't.) Also "data" is kind of vague... Actually, though, we could probably remove these comments entirely; they're tending in the direction of "Increment $i." ;) If anything, I'd like the comment to explain how the second time we see this line is different from the first.

  18. +++ b/core/modules/node/node.moduleundefined
    @@ -707,65 +733,99 @@ function _node_types_build($rebuild = FALSE) {
    +  // Sync the {node_type} table against the enabled modules list. Any node type
    +  // with a base other than 'node_content' inherits its disabled state from that
    +  // of its module.
    

    Ideally we should spell out "synchronize" here.

  19. +++ b/core/modules/node/node.moduleundefined
    @@ -707,65 +733,99 @@ function _node_types_build($rebuild = FALSE) {
    +      // A hook_node_info() was unavailable for the current occurence from the
    +      // database, so enable or disable the occurence depending on its module's
    +      // state.
    

    I don't quite understand this comment. What does it mean for a hook to be "unavailable"? Just that there are no implementations of it? What "occurrences" of what?

  20. +++ b/core/modules/node/node.moduleundefined
    @@ -707,65 +733,99 @@ function _node_types_build($rebuild = FALSE) {
    +    // Don't litter the return data structure with internal data.
    

    We don't do not use contractions in our UI and API text. :)

  21. +++ b/core/modules/node/node.moduleundefined
    @@ -707,65 +733,99 @@ function _node_types_build($rebuild = FALSE) {
    +        // Explicitly remove the new_state flag or someone will start depending
    +        // on unspecified behavior.
    

    This comment is... odd. In general it's best not to refer to mysterious "yous" and "wes" and "someones" in our UI and API text. (Also, should we single-quote the flag name?) Maybe:
    Explicitly remove the 'new_state' flag; it is for internal API use only.

    Is that accurate? What do you think?

  22. +++ b/core/modules/node/node.moduleundefined
    @@ -1256,14 +1316,14 @@ function node_delete_multiple($nids) {
    -        // because node module is implementing search module's API, not the other
    -        // way around.
    +        // because node module is implementing search module's API, not the
    +        // other way around.
    ...
    -      // Delete after calling hooks so that they can query node tables as needed.
    +      // Delete after calling hooks so they can query node tables as needed.
    
    +++ b/core/modules/node/node.testundefined
    @@ -521,7 +521,7 @@ class NodeCreationTestCase extends NodeWebTestCase {
    -   * Create a page node and verify that a transaction rolls back the failed creation
    +   * Create page node and verify that transactions roll back failed creations.
    

    I think these cleanups are out of scope? So it would be best to leave it out of the patch and fix it in another issue. (There are several things that were missed in the node.module API cleanup patch, but I can't remember if we opened the followup or not... we should double-check before opening a new followup.)

  23. +++ b/core/modules/node/node.moduleundefined
    @@ -3719,7 +3779,8 @@ function _node_access_rebuild_batch_finished($success, $results, $operations) {
    +  // It is impossible to define a content type with a base that is not
    +  // node_content without also implementing hook_form().
    

    I think we want 'node_content' in single quotes here, too (for consistency).

  24. +++ b/core/modules/node/node.testundefined
    @@ -1447,6 +1447,32 @@ class NodeTypeTestCase extends NodeWebTestCase {
    +   * Tests handling of node type types that are undefined.
    

    Node type types? Node types typo? :) Also, "Tests the handling..."

  25. +++ b/core/modules/node/node.testundefined
    @@ -1447,6 +1447,32 @@ class NodeTypeTestCase extends NodeWebTestCase {
    +    $this->assertTrue($info !== FALSE, 'Return data from node_type_get_type is valid for a disabled content type with a base that is not node_content.');
    ...
    +    $this->assertTrue($info !== FALSE, 'Return data from node_type_get_type is valid for a disabled content type with base of node_content.');
    

    Function names should include () in documentation and UI text (i.e., node_type_get_type()).

Also, please include an interdiff of your changes if possible so we can review the improvements easily and make sure we're not accidentally changing anything else. Thanks!

(I didn't realize how complicated the documentation here actually is when I reviewed the patch previously. Phew!)

xjm’s picture

+++ b/core/modules/node/node.moduleundefined
@@ -669,30 +686,39 @@ function node_type_update_nodes($old_type, $type) {
+  * This function provides the {node_type} table state as it would appear after
+  * a rebuild, actually rebuilding the table when $rebuild is true. The returned
+  * node type's disabled property indicates if the node type's implementing
+  * module has been disabled, however, any node type with a base value of
+  * 'node_content' always remains non-disabled, making Node Content a
+  * bad name for a module.

I just realized I posted a separate comment for every line of this paragraph... my bad. Gimme a minute and I'll just rewrite it to what I think it should be.

xjm’s picture

Alright, so ignore points 6-9 in #55 above and just replace the paragraph with something like:

This function provides the {node_type} table state as it would appear after a rebuild. When the $rebuild parameter is set to TRUE, the table is also actually rebuilt. The returned node type's 'disabled' property indicates whether the module that defines it has been disabled. However, any node type with a value of 'node_content' for the base will never be disabled.

Does that seem clear/make sense? Further improvements encouraged, since this is pretty information-dense. :P

chx’s picture

xjm asked me to come in and say the entire approach is wrong to give ryanissamson the Whole Drupal Core Experience. Sorry, I can't say that but I can say I dislike the $map variable name, let's call it $t_arguments. Thanks for the work.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
15.24 KB

This doesn't address any of the concerns from #55-#58, except the confusing duplication of the lines documented // Copy data to names array., as mentioned in #55, part 17.

Just seeing if this still passes.

tim.plunkett’s picture

Status: Needs review » Needs work

Okay, still CNW for #55-58. Note for when rerolling it, I accidentally included some test code in #59, the real interdiff is

--- a/core/modules/node/node.module
+++ b/core/modules/node/node.module
@@ -751,9 +751,6 @@ function _node_types_build($rebuild = FALSE) {
       if ($rebuild) {
         $active_types->types[$type]->new_state = TRUE;
       }
-
-      // Copy data to the names array.
-      $active_types->names[$type] = $active_types->types[$type]->name;
     }
   }

This is first of the two time $active_types->names[$type] is set, and its completely unnecessary.

ryan.gibson’s picture

Here's the latest and greatest (with interdiff I might add).

XJM, comment #19 from your remarks threw me for a loop; I'm not quite sure what the original patch creator was trying to say there.

ryan.gibson’s picture

Status: Needs work » Needs review
xjm’s picture

Alright, thanks @ryanissamson. It looks like everything from my post above has been covered other than this bit:

+++ b/core/modules/node/node.moduleundefined
@@ -707,65 +733,99 @@ function _node_types_build($rebuild = FALSE) {
+      // A hook_node_info() was unavailable for the current occurence from the
+      // database, so enable or disable the occurence depending on its module's
+      // state.

Two other observations about things I think are still unclear:

+++ b/core/modules/node/node.moduleundefined
@@ -515,13 +515,13 @@ function node_type_save($info) {
+    // However, when the module is disabled, the [base]_form is hidden if it was

Hmmm, still don't know what it means for it to be "hidden."

+++ b/core/modules/node/node.moduleundefined
@@ -690,34 +690,33 @@ function node_type_update_nodes($old_type, $type) {
+  *     $rebuild is equal to TRUE. The property is a thin version of the 'types'

What does "a thin version" mean?

Finally, chx's point from #58 still hasn't been addressed.

None of these things are blockers for the issue; I think the patch is at least comprehensible now. :)

ryan.gibson’s picture

Changes suggested by chx in this patch.

catch’s picture

Sorry coming in a bit late, but

+ * Modules should not create node types with this function, but should use
+ * hook_node_info() instead.
+ *

Why?

http://api.drupal.org/api/drupal/modules%21node%21node.module/function/c...

xjm’s picture

Issue tags: -Novice

I think we've cleaned up all the novice things.

ryan.gibson’s picture

Assigned: ryan.gibson » Unassigned
kristiaanvandeneynde’s picture

Issue tags: +Needs documentation

is_new flag?

There seems to be an issue with the is_new flag. According to the new documentation, it should only get set to TRUE if it comes from hook_node_info() and was not in the node_type table yet. However, the patch in #64 adds the is_new flag to all node types defined in hook_node_info().

+    foreach (module_invoke($module, 'node_info') as $type => $node_info) {
+      $node_info['type'] = $type;
+      $active_types->types[$type] = node_type_set_defaults($node_info);
+      $active_types->types[$type]->module = $module;
+
+      // Overwrite the defaults of 'is_new' and 'disabled' for the new active
+      // type.
+      $active_types->types[$type]->is_new = TRUE;


node_type_access query tag?

There also seems to be an oversight (I think?) in _node_types_build()'s query tags. It says ->addTag('node_type_access'), where all other queries in node.module set ->addTag('node_access'). There isn't even an implementation of hook_query_TAG_alter() for node_type_access, while there is for node_access: node_query_node_access_alter(). If this was intended, it could use some inline documentation?

@catch (#65)

The D7/D8 book module actually "does it wrong". We should encourage module developers to do it like the D7 blog module does: A small .install footprint and using hook_node_info(). See book.install vs blog.install

From hook_node_info():

Only module-provided node types should be defined through this hook. User-provided (or 'custom') node types should be defined only in the 'node_type' database table, and should be maintained by using the node_type_save() and node_type_delete() functions.

The problem is that you can hardly find any decent information on how to programmatically create a node type in Drupal. Even the Examples module completely ignores hook_node_info in favor of calling node_type_save() and node_type_delete() itself.

IMO: We should add a page to the module developers guide to set a standard on how to create a node type, preferably using hook_node_info() and discouraging base => 'node_content'. That page would also have to have a clear explanation on how to implement hook_form(), hook_insert(), etc. Sadly enough, hook_form and the likes aren't really hooks to begin with; they're dynamic callbacks. Some of which are to be deleted (see #1390716: kill hook_form() with fire).

The biggest problem, however, is that the "wrong" approach as seen in the Examples module is actually the only approach so far that properly cleans up nodes and node types after uninstalling the module. If we want to encourage people to use hook_node_info(), we need to get a garbage disposal going for nodes of a deleted/disabled node type.

In short: the whole node_type_save()/node_type_delete() vs hook_node_info() story deserves an issue of its own.
Edited this comment to separate on-topic from off-topic.

tim.plunkett’s picture

FileSize
13.92 KB

Some of the hunks here were obsoleted by #1361234: Make the node entity a classed object and #1404198: Separate database cache clearing from static cache clearing and data structure rebuilding, and the tests needed a post-PSR-0 reroll.

I did not address anything in #68.

Status: Needs review » Needs work

The last submitted patch, drupal-1268974-69.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
14.03 KB

Added 'use stdClass;'

Status: Needs review » Needs work

The last submitted patch, drupal-1268974-71.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
14.02 KB
Anonymous’s picture

Notice that there is a small type in the reproduce steps, the function in question is 'node_type_get_types', with an extras 's'.

Actually I'm not sure. That function doesn't take any arguments. But then I again, I can't find a function reference to node_get_node_type anywhere in the Drupal 8 source.

iainp999’s picture

I think node_type_get_types will do for drush ev. We can then just check for an array entry for our type? (or lack of array entry).

Anonymous’s picture

Yeah, just run 'drush eval "node_type_get_types();"' and check for the types in the list.

iainp999’s picture

Status: Needs work » Needs review
FileSize
13.99 KB

So the last patch didn't apply cleanly to the latest code; I reckon this is due to conflicts with code that has been committed since the patch.

I've manually added the conflicting changes as per the patch. EDIT: The new test passes, looking into why a separate test failed automated testing.

Status: Needs review » Needs work

The last submitted patch, drupal-1268974-76.patch, failed testing.

iainp999’s picture

Running the whole test set in dev to try to replicate the single test failure from the last patch.

andypost’s picture

Status: Needs review » Needs work
tim.plunkett’s picture

Version: 8.x-dev » 7.x-dev
tim.plunkett’s picture

Issue summary: View changes

Updating issue summary