Put node_get_types() out of its misery
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | node system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
| Issue tags: | DX (Developer Experience), Needs Documentation, Needs tests |
A quote from http://api.drupal.org/api/function/node_get_types/6:
Parameters
$op The format in which to return the list. When this is set to 'type', 'module', or 'name', only the specified node type is returned. When set to 'types' or 'names', all node types are returned.
$node A node object, array, or string that indicates the node type to return. Leave at default value (NULL) to return a list of all node types.
...
Return value
Either an array of all available node types, or a single node type, in a variable format. Returns FALSE if the node type is not found.
This function is basically trying to do no less than about 5 different things, depending on what combination of data types is passed in. That's completely insane. We need to either split this up into multiple API functions, or at least rename it to node_get_info() or something.
#1
subscribe
#2
My initial view would that we should seperate into multiple API functions. As this will make the API more descret and clear to devs with regards what return to expect.
The following combinations are actually used by core
node_get_types('type', $node)node_get_types('type', $node->type)
node_get_types('names')
node_get_types('name', $node)
node_get_types()
node_get_types('types', NULL, TRUE)
node_get_types('module', $node)
So maybe we should move towards having the following APIS
function node_get_types($reset = false)
function node_get_module($node)
function node_get_names()
function node_get_name($node)
function node_get_type($node) or function node_get_type($node->type)
There maybe other combinations required but if core doesn't use them I see that as unlikely. Not keen on the last signature so maybe it needs a rethink. Mind you I'll simply putting an initial idea out there!!
#3
I have had a look at this a bit more and in the case of
node_get_types('type', $node)node_get_types('type', $node->type)
We are only interested in the node type so the last function can become
function node_get_type($type)where type is node->type or node['type'].
In addition
function node_get_module($node)function node_get_name($node)
Can become
function node_get_module($type)function node_get_name($type)
where type is node->type or node['type'].
The only draw back of this aproach is potentially have multiple static variables which contain the variables. So an alternative would be (where type is node->type or node['type']):
function node_get_types($op = 'types', $type = NULL, $reset = FALSE) {
static $_node_types, $_node_names;
if ($reset || !isset($_node_types)) {
list($_node_types, $_node_names) = _node_types_build();
}
if ($type) {
if (!isset($_node_types[$type])) {
return FALSE;
}
}
switch ($op) {
case 'types':
return $_node_types;
case 'type':
return isset($_node_types[$type]) ? $_node_types[$type] : FALSE;
case 'module':
return isset($_node_types[$type]->module) ? $_node_types[$type]->module : FALSE;
case 'names':
return $_node_names;
case 'name':
return isset($_node_names[$type]) ? $_node_names[$type] : FALSE;
}
}
I'll look at rolling an initial patch once I get an idea of the appraoch we want to take.
#4
We could put a wrapper function around the static variable, like so:
function _node_types() {static $_node_types, $_node_names;
if ($reset || !isset($_node_types)) {
list($_node_types, $_node_names) = _node_types_build();
}
return $_node_types;
}
That way you don't have to maintain static variables across each function.
#5
Yes that makes sense then we get the benefit of specific functions for specfic requests plus a single array. This function only returns $_node_types and not $_node_names. We only ever want either $_node_types or $_node_names so
function _node_types($bytype = true) {static $_node_types, $_node_names;
if ($reset || !isset($_node_types)) {
list($_node_types, $_node_names) = _node_types_build();
}
if($bytype){
return $_node_types;
}
else{
return $_node_names;
}
}
#6
Wouldn't it make sense to move the caching part (static variables) to _node_types_build() ? In that case it could have a function definition something like :
<?php
function _node_types_build($reset = TRUE) {
static $_node_types, $_node_names;
if ($reset == TRUE || !isset($_node_types)) {
$info_array = module_invoke_all('node_info');
foreach ($info_array as $type => $info) {
$info['type'] = $type;
$_node_types[$type] = (object) _node_type_set_defaults($info);
$_node_names[$type] = $info['name'];
}
$type_result = db_query(db_rewrite_sql('SELECT nt.type, nt.* FROM {node_type} nt ORDER BY nt.type ASC', 'nt', 'type'));
while ($type_object = db_fetch_object($type_result)) {
// Check for node types from disabled modules and mark their types for removal.
// Types defined by the node module in the database (rather than by a separate
// module using hook_node_info) have a module value of 'node'.
if ($type_object->module != 'node' && empty($info_array[$type_object->type])) {
$type_object->disabled = TRUE;
}
if (!isset($_node_types[$type_object->type]) || $type_object->modified) {
$_node_types[$type_object->type] = $type_object;
$_node_names[$type_object->type] = $type_object->name;
if ($type_object->type != $type_object->orig_type) {
unset($_node_types[$type_object->orig_type]);
unset($_node_names[$type_object->orig_type]);
}
}
}
asort($_node_names);
}
return array($_node_types, $_node_names);
}
?>
By default, without any arguments, it would build the list of available node types, like it's doing now. In the new functions like node_get_name, they could call _node_types_build accordingly. This way, imho the cache is kept where it belongs.
Spreading the cache over several functions like marcingy said is indeed something you don't want (duplication, use of multiple static variables storing the same information).
A function wrapper like jgoldberg mentioned is possible, but personally I don't feel much for introducing another function just to keep track of the static variables, plus it would mean yet another function call (a tiny bit more performance cost).
The only objection perhaps against having _node_types_build keeping track of the cache is that the function name would be a bit more fuzzy.
#7
Yeah, when I first looked at the code, I remember thinking, "Why are the static definitions in node_get_types, not _node_types_build?"
#8
Makes total sense. I'll look to get an initial patch rolled this weekend(likely on Sunday).
#9
Ok for now I have kept the code in which checks whether $node is an array, object or string because some calling functions have mixed types when calling.
Initial patch for review
#10
#11
Version 2 tidy up some formatting and removed the isset check for $node. We always have a node it isn't an optional parameter we just need to know if it is array,string or object.
#12
Version 3.
Moved checking of what type $node is to a help function which always returns a string. Corrected parameter information. Remove the 2 additional calls to _node_types_build() in node_types_rebuild(). I can't see a reason for thes calls if there is I'll gladdly put them back.
#13
/**
@@ -450,9 +487,7 @@
* module-defined node types to the database.
*/
function node_types_rebuild() {
- _node_types_build();
-
- $node_types = node_get_types('types', NULL, TRUE);
+ $node_types = node_get_types(TRUE);
foreach ($node_types as $type => $info) {
if (!empty($info->is_new)) {
@@ -462,8 +497,6 @@
node_type_delete($info->type);
}
}
-
- _node_types_build();
}
I think we need that second _node_types_build() since we are adding/removing nodes.
#14
Also, this is minor, but you rolled your .project file into the patch.
#15
New patch with call back in and .project removed.
#16
I haven't had any problems with this patch. It's a fairly simple refactoring. Looks good.
#17
If we are rollin' back my patch which created this function a couple years ago, I will bow to the will of the community. But. Please don't add code duplication to core, thanks.
#18
I understand the concerns about duplication of code, but at the same time the function as Webchick originally stated does return node types alone so maybe we just need to rename the function if we want to reduce duplication.
#19
How about option C): keep the separate functions, but make them wrappers around some internal function named something like _node_get_info() that's never intended to be called by external modules/developers, and can take all the crazy-stupid parameters and handles the FALSE checking and whatnot.
I get that chx doesn't want code duplication, but OTOH I think it's ridiculous that you need API docs in front of you to figure out how to get basic node information.
#20
Oops. Didn't mean to reset status.
#21
I'll roll a new patch using that approach and throw it back out for consultation.
#22
Yeah, that's probably the best solution.
#23
New version. Restored the original node_get_types function and rename it to _node_get_types.
Provided new api for modules to use which wraps _node_get_types.
#24
I can think of three decent ways to make the node_get_types() API easier to use:
(a) node_get_types() becomes _node_get_types(), and we introduce public wrapper functions that have meaningful names for module developers.
(b) change the node-type API to a class, and make $_node_types and $_node_names members (i.e. static variables) of that class.
(c) change $_node_types and $_node_names to be global variables - then we can remove _node_get_types() altogether, and just have the new meaningfully-named API functions.
Option (c) would appeal to me the most - global variables are the easiest way to avoid duplicate data storage - except that global variables are also evil, and that adding more to core is insane.
Option (b) would solve the problem similarly to (c), but would avoid using evil globals. However, I'm not particularly attracted to the idea of transforming an important part of the node system into a class at this stage.
So I think (a) is the most attractive, and I see that #23 implements (a). So +1 to the above patch.
#25
I'm all for cleaning this up. As webchick said, it's ridiculous that you need API docs in front of you to figure out how to get basic node information. Any other clean-up that can be done as part of this patch?
The patch in #23 needs work -- there are coding style issues.
#26
#27
Correct code style issues
#28
#29
With a nod towards http://www.tejasa.com/node/123
#30
Nice :)
#31
The test for the issue (more like a feature request IMHO)
Patch #27 pass the test. =)
<?php
class TestCase220233 extends DrupalTestCase {
function get_info() {
return array(
'name' => t('[220233] Put node_get_types() out of its misery'),
'desc' => t('This function is basically trying to do no less than about 5 different things, depending on what combination of data types is passed in. That\'s completely insane. We need to either split this up into multiple API functions, or at least rename it to node_get_info() or something.'),
'group' => t('Drupal 7 Tests'),
);
}
// This is not 'a test' actually, just a test of presence
function testIssue() {
$this->assertTrue(function_exists('node_get_types'), t('Check on existence of node_get_types()'));
$this->assertTrue(function_exists('node_get_type'), t('Check on existence of node_get_type()'));
$this->assertTrue(function_exists('node_get_names'), t('Check on existence of node_get_names()'));
$this->assertTrue(function_exists('node_get_name'), t('Check on existence of node_get_name()'));
$this->assertTrue(function_exists('node_get_module'), t('Check on existence of node_get_module()'));
$this->assertTrue(function_exists('_node_get_types'), t('Check on existence of _node_get_types()'));
}
}
?>
#32
The patch in #27 still has some code style issues, include initial capitalization (and in one case, an ending period) in about five docblocks similar in format to this:
+ /**+ * returns false if the node name does not exist. Otherwise returns the nodes name.
+ *
+ * @param $node
+ * A node object, array, or string that indicates the node type to return.
+ */
Plus, in most of the comments in question, words like "nodes" could really use an apostrophe "s".
#33
New version correct typos in comments and added return values for all functions
#34
Re-rolled patch from #33 for current HEAD, with some fixes to block comment indentation pointed out by #32.
#35
This patch doesn't eliminate the problem. _node_get_types still does about 5 things. It just attempts to cover up the problem. What we really need is to split the code out into different functions, that are not just wrappers. We can have one internal function that holds an array with all the data, and just returns it when called. Then the other functions can extract data from that array.
#36
See patch in #15 which was the original solution that was dropped after some comments from other developers. That might make for the basis of your idea.
#37
node_get_name($node)??While I wholeheartedly agree with webchick that the original function is confusing because of the myriad ways in which we can use it, I believe that most of the new function names proposed in the latest patch don't make much sense semantically. They do make sense only to those who will remember the original function API.
Who would guess that node_get_name($node) will get you the human-readable name of the content type...?
#38
The last submitted patch failed testing.
#39
Shouldn't node_get_types (or whatever it is being renamed to now) also have an option to return node names and types...?
array(
"type" => "name"
);
#40
#41
#42
Patch no long applies against current HEAD.
#43
Here's my re-roll. I agree that flaws still exist. Setting to CNR for the testing bot.
#44
The last submitted patch failed testing.
#45
Reroll of #43 against head
#46
The last submitted patch failed testing.
#47
The patch needed several changes, because of fields, the "module" -> "base" rename and other stuff.
Patch 9 is just a more or less simple re-roll of the last patch. I renamed node_get_module() to node_get_base() and renamed all (new) occurences of that and old-style calls to node_get_types().
However, I don't really like the approach of that patch, because it simply hides the complexity of _node_get_types(), it does not remove it.
Because of that, I tried to create a similiar approach as the first series of patches. We want those wrapper functions anyway, so it seems to be less code duplication when we extend *some* of those with 1-2 lines, instead of calling that big, hard to understand _node_get_types() function all the time. My ideas:
- refactor the $node to $type conversion to a simple function _node_extract_type(). Should be the same code as in the first patches.
- Moved the static cache handling to _node_types_build().
- Converted the static cache to a object with properties types and names instead of two variables. This has the simple advantage that you can directly access the needed part of the data on a single line, which is not possible with arrays. for example: "_node_types_build()->names". I am not sure how this influences speed, but I don't think it is slower than list() and so on.
With the above changes, the complexest of these functions looks like this:
<?phpfunction node_get_base($node) {
$type = _node_extract_type($node);
$types = _node_types_build()->types;
return isset($types[$type]) && isset($types[$type]->base) ? $types[$type]->base : FALSE;
}
?>
And node_get_names/types just contain a single line of code, for example:
<?phpfunction node_get_names() {
return _node_types_build()->names;
}
?>
These changes are part of patch 10.
Questions:
- I am *really* wondering if we need _node_extract_type() at all. This just doesn't make sense to me, because the caller of node_get_xy() should always know if he has a node object or just the type string, and I'm pretty sure the array syntax isn't used at all in core anymore. As we are changing these functions anyway, why not simply require $type? It should be easy to convert all $node parameters to $node->type, it's already done at several places. This would save another line in these functions and that function could be removed.
- I am also unsure about the comment in #13, because node_get_types(TRUE) will call _node_types_build() internally, in both patches.
- I'd also like to get some feedback about the (object) idea
- It seems a bit hard to understand the difference between name/type, especially as the function is now called node_get_name(), which returns the name of the type. See #37
I will try to upload both patches now, I have no idea what the test bot will do with these. Both *should* pass the tests.
#48
Working on a new patch which tries to remove node_extract_type() and enforces $type for the new functions.
This is bigger than I thought first, I still think this should be done but probably as a follow-up patch. The reason for that is that several functions have that object/array/string kind of parameter and simply pass that through node_get_type/name/base which takes care of it.
Detailed Changes:
- nook_hook changed to $type. Befor the change, it even passed $node as by reference, but I can't see a reason for this, because $node can never be changed. I assume this is because of copy/paste.
- node_invoke changed to $node (object). It *seems* this function is always called with $node (object), but I'm not 100% sure. But atleast I cant find a call that isn't and the tests pass.
- node_list_permissions() is declared as $type string, but was called with $type object in one case, fixed in the caller.
- node_access is sometimes called with the node type and sometimes with a node object. Added a check and call node_get_base with either $node->type or $node.
- Replaced all node_get_name/base/type($node) and similar calls with $node->type
I am uploading the patch for reference, but we might want to continue with patch 10.
#49
The last submitted patch failed testing.
#50
Forgot to convert a node_hook() call.
#51
Updated based on patch 10. _node_extract_type removal is most probably a follow-up issue, too many things need to be changed because of that.
- renamed node_get_X() functions to node_type_get_X().
- fixed some apidoc things.
- removed the explicit _node_types_build() call in node_types_rebuld(), as it doesn't make any sense.
- created a new NodeTypeTestCase, which contains only a few simple node_type_get_* function validation for now and can be extended later.
- moved the new functions a bit to make the review easier.
#52
The last submitted patch failed testing.
#53
Re-roll:
- Updated _node_types_build() to drupal_static() and simplified the cache handling a bit.
#54
Updating the status.
#55
I like the API names and not going to open a bikeshedding why you used ->types and ->names instead of [types] and [names] inside an internal function. Let's get this out the door instead.
#56
Holy cow! This patch is flambéed awesome sticks braised in awesome sauce and served with a refreshing glass of awesome juice.
Couple of minor nits, and then hopefully a commit. :)
+ drupal_static_reset('_node_types_build');(there are two of these)
Although it makes the eyeballs bleed, for better or worse we've standardized on + drupal_static_reset(__FUNCTION__); throughout the rest of core.
Oh. Nevermind, I get it. You're resetting _node_types_build's static cache from node_types_build(). Sneaky! Maybe add a comment there so people know this is done on purpose? Eh. I'm not sure. This whole part of the code is very confusing. Could we maybe rename _node_types_build() to something like node_types_cache() or something more indicative of what it's doing? I think that might help with the legibility here.
I noticed +function _node_extract_type($node) { no longer returns FALSE like the old code. Was that by design? This seems to hint at some larger inconsistencies in the API; +function node_type_get_type($node) { returns FALSE if the type is not found, but +function node_type_get_types() { doesn't. What's up with that? Could we unify this a bit? FALSE seems like it would be a useful return value on all of these if no valid value is found.
+ * @return+ * A list of node types, as an array.
Could we have a little bit more information about said array? Is it keyed by something? Does it contain objects or strings? etc.
+ * Returns the node type base of the passed node or node type string.I appreciate comment symmetry, but what in the heck is a 'base'? :) We could probably do with a second comment line that explains this a bit more so people know when/if they should use it.
+ function testNodeTypeGetFunctions() {The PHPDoc seems to indicate that this is going to be a test for all node_type_get_X functions, yet it seems to only test node_type_get_types() and node_type_get_names(). Is this because these are implicitly tested? If so, we should make a comment to that effect.
#57
> Maybe add a comment there so people know this is done on purpose?
I added comments to the drupal_static_reset() calls. I know this stuff is somewhat confusing and it's also heavily discussed because Dries doesn't like it. See #422378-7: convert theme.inc, unicode.inc, and xmlrpc.inc to use new static caching API. The name change is also an interesting idea...
> I noticed +function _node_extract_type($node) { no longer returns FALSE like the old code.
The function has been added newly by this patch and needs to return a string. It is needed because the node_type_get_* functions don't know what you pass in so they need to figure this out. I want to kill this function in a follow-up and always call the node_type_get_* function with $type.
> +function node_type_get_type($node) { returns FALSE if the type is not found, but +function node_type_get_types()
get_type returns FALSE when the specified node type doesn't exist. get_types will always return an array with 0-n node types in it. I don't think it's a good idea to return FALSE if there are no node types (which should not happen that often :)) because the function is often used together with foreach. Also, FALSE for get_type() indicates that the call has "failed", because you expected the type exists and now it doesn't. This is not the same for get_types().
> Could we have a little bit more information about said array? Is it keyed by something? Does it contain objects or strings? etc.
Improved comment and added an example what each node type contains.
> I appreciate comment symmetry, but what in the heck is a 'base'? :)
Well, when I wrote that line, I hadn't any idea of what that is either :) I tried to add an explanation.
> The PHPDoc seems to indicate that this is going to be a test for all node_type_get_X functions, yet it seems to only test node_type_get_types() and node_type_get_names()
The test actually only "test" the other functions, because they assume that the data return by get_names and get_types() is correct. They simply compare that "global" data, built by _node_types_build(), to the specific data returned get_type(), get_name() and get_base(). I'm not sure if we should test more (in this patch).
#58
#59
- 'bundles' => node_get_types('names'),+ 'bundles' => node_get_names(),
should be node_type_get_names() ?
[edit: that's the one in field.api.php. The actual code in node.module correctly uses node_type_get_names()]
#60
Minor: 3 occurrences of
+ $types = _node_types_build()->names;(double spacing)
#61
Oh, there were quite a few instances where "_type" was missing, updated...
#62
Crosspost with #60, should be fixed now too.
#63
The last submitted patch failed testing.
#64
Re-roll. no changes...
#65
The last submitted patch failed testing.
#66
Re-roll, no changes.
#67
The last submitted patch failed testing.
#68
There was a php syntax error in hook_node_build_alter, no idea how that could happen :)
Re-roll with syntax error fixed...
#69
The last submitted patch failed testing.
#70
Re-roll, the error has been fixed.
#71
The last submitted patch failed testing.
#72
And another re-roll :)
The remaining question is regarding the static cache. Should I re-introduce the $reset parameter to _node_build_types()? I think not, because using drupal_static_reset() allows us to reset the cache without forcing a rebuild of the cache.
I think I've resolved the other things pointed out by webchick at #56.
#73
The last submitted patch failed testing.
#74
Fixed a typo...
#75
The last submitted patch failed testing.
#76
re-roll
#77
The last submitted patch failed testing.
#78
There was a new node_get_types() call which I've forgot to convert.
#79
The last submitted patch failed testing.
#80
Implemented a node_type_clear() method, which is similiar to form_clear_error() (see #422362: convert form.inc to use new static caching API).
#81
$node is always either a object or a string, removed the is_array check and updated the apidocs.
#82
I would think this is OK. All our concerns seem to be addressed.
#83
I'm way, way too tired to commit this right now, but bumping because I would love to commit it TOMORROW! :)
#84
I looked at this for a 10-15 minutes and it looked good. Go and commit it, webchick!
#85
Yay!! Committed to HEAD! :D
Marking "needs work" until this is updated in the module upgrade docs.
#86
http://drupal.org/node/224333#node_type_get_functions
Is that enough? (Note that the api doc link currently doesn't work, because they are not yet updated..)
#87
Looks good to me.
#88
To make it easier to see what happened, I am crosslinking #62340: Pave the way for CCK
#89
Ups...
Looks I made a mistake, there is a missing &.
#90
Committed that as well. Thanks!
#91
Automatically closed -- issue fixed for 2 weeks with no activity.