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:
- Install Drupal 8.
- Enable the book module.
- Disable the book module.
- Verify that drush eval "print_r(node_type_get_type('book'));" returns an object.
Steps to reproduce a failing result:
- Write a small module that just implements hook_node_info() with a base that is not "node_content" (example implementation below).
- Enable your module.
- DIsable your module.
- 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.
Comment | File | Size | Author |
---|---|---|---|
#77 | drupal-1268974-76.patch | 13.99 KB | iainp999 |
#73 | drupal-1268974-73.patch | 14.02 KB | tim.plunkett |
#71 | drupal-1268974-71.patch | 14.03 KB | tim.plunkett |
#69 | drupal-1268974-69.patch | 13.92 KB | tim.plunkett |
#64 | 1268974-cleanup_and_fix_errors-64.patch | 15.17 KB | ryan.gibson |
Comments
Comment #1
marcingy CreditAttribution: marcingy commentedThis does not make drupal usable.....
Comment #2
tpopham CreditAttribution: tpopham commentedWell 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.
Comment #3
tpopham CreditAttribution: tpopham commentedTry that again
Comment #5
tpopham CreditAttribution: tpopham commentedGiving 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.
Comment #7
tpopham CreditAttribution: tpopham commentedWell 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).
Comment #8
tpopham CreditAttribution: tpopham commentedMy 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.
Comment #9
tpopham CreditAttribution: tpopham commentedOops. I'm still new to this
Comment #10
tpopham CreditAttribution: tpopham commentedRemoving 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.
Comment #11
Mile23subscribe
Comment #12
tpopham CreditAttribution: tpopham commentedThe `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().
Comment #13
tpopham CreditAttribution: tpopham commentedThe 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).
Comment #14
tpopham CreditAttribution: tpopham commentedI 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.
Comment #15
tpopham CreditAttribution: tpopham commentedReworking 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.
Comment #16
tpopham CreditAttribution: tpopham commentedLooking 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.
Comment #17
tpopham CreditAttribution: tpopham commentedNever mind.
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.
Comment #18
tpopham CreditAttribution: tpopham commentedRereading the hook_node_info() docs:
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
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.Comment #19
tpopham CreditAttribution: tpopham commentedApologies to the Blog authors. The blog module implements its node type by hook_node_info() with base==blog, obtaining reasonable behavior.
Comment #20
catch@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.
Comment #21
tpopham CreditAttribution: tpopham commentedOf 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.
Comment #22
tpopham CreditAttribution: tpopham commentedI'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.
Comment #23
Mile23Putting this where eyes can see it....
Comment #24
catchWhile #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.
Comment #25
Mile23Google isn't my friend on this... Is there a way to uninstall a module in the test regime?
Comment #26
spydmobile CreditAttribution: spydmobile commentedHas anyone seen this?
http://andypangus.com/an-annoying-error-part-two
looks like it might be a potential D7 patch?
Franco
Comment #27
bdurbin CreditAttribution: bdurbin commentedAdding 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.
Comment #28
jhnbrnn CreditAttribution: jhnbrnn commentedRe-rolled the patch against 8.x.
Comment #29
magnusk CreditAttribution: magnusk commentedWe've updated the issue summary to match the standard.
Comment #30
magnusk CreditAttribution: magnusk commentedWe've checked that the test passes after re-rolling the patch.
Comment #31
bdurbin CreditAttribution: bdurbin commentedRe-rolled the test (syntax error [blerg]).
Comment #32
bdurbin CreditAttribution: bdurbin commentedRe-submitting tests.
Comment #34
bdurbin CreditAttribution: bdurbin commentedOur 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.
Comment #35
magnusk CreditAttribution: magnusk commentedShould 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.
Comment #36
xjmThanks @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:
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
Looks like this is missing the end of the sentence? :)
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:
The second sentence should be in a separate paragraph after a blank line. Reference: http://drupal.org/node/1354#functions
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
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
(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.
The word 'disabled' should be in single quotes, if anything (no opening backtick).
I don't think we need the editorial here in parentheses.
Let's not use the word "clobber."
Let's remove the parenthetical about the translatable tag. :)
I think we can just remove this comment.
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".
The word "iterant" is not commonly used; I'd recommend rewording this comment to be a bit simpler and clearer.
We don't need to reference the pre-patch behavior here; instead, it would be better to just explain the what/why more briefly.
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).
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!
Comment #37
ryan.gibson CreditAttribution: ryan.gibson commentedI'll take this one. :)
Comment #38
ryan.gibson CreditAttribution: ryan.gibson commentedHere'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.
Comment #39
ryan.gibson CreditAttribution: ryan.gibson commentedOops, changing to NR.
Comment #40
tim.plunkettMissing 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.
Missing trailing full stop.
Missing trailing full stop.
Indented 2 spaces too many
Missing trailing full stop.
Needs spaces on either side of ==
Missing trailing full stop.
Indented 2 spaces too many
Missing trailing full stop.
Indented 2 spaces too many
Missing trailing full stop.
Missing trailing full stop.
Should start with a capital letter.
Missing trailing full stop.
Missing trailing full stop.
Typo: undefined
Comment #41
ryan.gibson CreditAttribution: ryan.gibson commentedMade changes after the comments by @timplunkett.
Comment #42
tim.plunkettnode_type_delete should have ()
I think this should be @see _node_types_build()
This should be rewrapped to be as long as possible without going over 80 chars.
I think the the == should be replaced with English, and is that the right usage of "modular"?
Shouldn't use a backtick here, and I think this should be reworded to start the sentence with a capital.
See two comments above, about modular and ==.
Two spaces in here. Should "aren't" be "were not"? Or can this be reworded to not refer directly to a function?
spaces around ==
The curlies generally refer to a DB table, as in node_type at the beginning of the paragraph.
"poor" seems not assertive enough.
This needs to be indented one space further.
Rewrite to use English, not ==.
Rewrite to use English, not ==.
This should be reworded to start with a capital, and should be less pseudo-code and more English.
Should use TRUE, not 1
This should be reworded to start with a capital, and should be less pseudo-code and more English. Also, missing trailing full stop.
Should use FALSE, not 0
This should be reworded to start with a capital, and should be less pseudo-code and more English. Also, missing trailing full stop.
Indented one space too many
Should have {} around the table name.
Should have {} around the table name.
"with that from the database" doesn't sound right.
Missing trailing full stop.
Now these are outdented too far.
Missing trailing full stop
Use FALSE, not 0
Now these are outdented too far.
Use FALSE, not 0
Now these are outdented too far.
Use FALSE, not 0
This looks like it's wrapped too early.
This should be rewritten to be under 80 chars.
undefined is now spelled wrongly a different way.
TRUE, not 1
TRUE, not 1
Comment #43
ryan.gibson CreditAttribution: ryan.gibson commentedHere 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. ;)
Comment #44
tim.plunkettLast pass, I think.
This should be below the @return, with a blank line before it, and just
@see _node_types_build()
, no punctuation or strings after it.'{Content types}' looks wrong to me. Probably just 'content types'
Missed one instance of rewriting the == to English.
node_content should likely be in single quotes, and put a comma after non-disabled
{node_type} is singluar
{types} should be wrapped in single quotes, not curly braces
Same, replace { with '
I guess the 0s and 1s here should be FALSE and TRUE. Sorry.
Overwrite is spelled wrong.
Wrap node_content in single quotes.
occurrence is spelled wrong twice
Comment #45
ryan.gibson CreditAttribution: ryan.gibson commentedAnd made those changes.
Comment #46
magnusk CreditAttribution: magnusk commentedComment #48
xjmGo, bot, go!
Comment #49
xjmWhoops. I think there may be an issue with QA. Retesting the patch.
Comment #51
xjmAlright, 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:
Comment #52
xjmThis is line 527 with the patch applied:
There are single quotes outside double quotes with additional single quotes in the string. That'd be the problem. ;)
Comment #53
ryan.gibson CreditAttribution: ryan.gibson commentedAlright, removed the single quotes.
Comment #54
ryan.gibson CreditAttribution: ryan.gibson commentedComment #55
xjmAwesome 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. :)
Missing asterisk here on the blank line.
I think we should say either "create node types" or "define node types." "Implement" does not seem correct in this context.
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.
Is it really a "parent" module? Maybe we can just say "The content type %type will not be disabled"?
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. :)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.
Nitpick: "if" should probably be replaced with "whether" here.
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...
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...
We should have articles here ("An associative array...").
TRUE should be in all caps here.
I'd say "is always FALSE" rather than "does not vary from FALSE."
Was this another zero that should be replaced with FALSE? (I'm not positive; please double-check.) :)
I'd say "that do not yet appear in the {node_type} table." (The last bit is confusing, and I think it's implied.)
Should we single-quote 'is_new' and 'disabled'?
I think the second comma here is incorrect.
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.
Ideally we should spell out "synchronize" here.
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?
We
don'tdo not use contractions in our UI and API text. :)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?
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.)
I think we want 'node_content' in single quotes here, too (for consistency).
Node type types? Node types typo? :) Also, "Tests the handling..."
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!)
Comment #56
xjmI 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.
Comment #57
xjmAlright, so ignore points 6-9 in #55 above and just replace the paragraph with something like:
Does that seem clear/make sense? Further improvements encouraged, since this is pretty information-dense. :P
Comment #58
chx CreditAttribution: chx commentedxjm 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.
Comment #59
tim.plunkettThis 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.
Comment #60
tim.plunkettOkay, still CNW for #55-58. Note for when rerolling it, I accidentally included some test code in #59, the real interdiff is
This is first of the two time
$active_types->names[$type]
is set, and its completely unnecessary.Comment #61
ryan.gibson CreditAttribution: ryan.gibson commentedHere'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.
Comment #62
ryan.gibson CreditAttribution: ryan.gibson commentedComment #63
xjmAlright, thanks @ryanissamson. It looks like everything from my post above has been covered other than this bit:
Two other observations about things I think are still unclear:
Hmmm, still don't know what it means for it to be "hidden."
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. :)
Comment #64
ryan.gibson CreditAttribution: ryan.gibson commentedChanges suggested by chx in this patch.
Comment #65
catchSorry coming in a bit late, but
Why?
http://api.drupal.org/api/drupal/modules%21node%21node.module/function/c...
Comment #66
xjmI think we've cleaned up all the novice things.
Comment #67
ryan.gibson CreditAttribution: ryan.gibson commentedComment #68
kristiaanvandeneyndeis_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().
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():
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.
Comment #69
tim.plunkettSome 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.
Comment #71
tim.plunkettAdded 'use stdClass;'
Comment #73
tim.plunkettUpdated per http://drupal.org/node/1574670
Comment #74
Anonymous (not verified) CreditAttribution: Anonymous commentedNotice 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.
Comment #75
iainp999 CreditAttribution: iainp999 commentedI 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).
Comment #76
Anonymous (not verified) CreditAttribution: Anonymous commentedYeah, just run 'drush eval "node_type_get_types();"' and check for the types in the list.
Comment #77
iainp999 CreditAttribution: iainp999 commentedSo 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.
Comment #79
iainp999 CreditAttribution: iainp999 commentedRunning the whole test set in dev to try to replicate the single test failure from the last patch.
Comment #80
andypostThis will need reroll after #111715: Convert node/content types into configuration
Comment #81
tim.plunkettNo longer valid for D8 after #111715: Convert node/content types into configuration
Comment #81.0
tim.plunkettUpdating issue summary