Posted by pwolanin on January 4, 2008 at 12:13am
| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | node system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
At least one bug I found a fix for ealier (link when I can find it) was due to sloppy use of the node type 'module' addribute.
The problem: for a type created by the node module, the 'module' attribute is set to 'node'. However this causes name-space conflict when invoking hook_node, etc, so in many places it is coerced as follows:
if ($module == 'node') {
$module = 'node_content'; // Avoid function name collisions.
}Why not simply store it as 'node_content' in the database? This would avoid having to special-case these and would make the code less prone to bugs in the future. Attached patch implements this and supplies an update function.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| node-type-namespace.patch | 4.06 KB | Ignored: Check issue status. | None | None |
Comments
#1
"Atributes" is not spelled "Atributes", but rather "Attributes". So, cnw.
#2
fixed typo
#3
better- I missed the profile
#4
#5
#6
bump
#7
fails on HEAD (7.x)
#8
Re-rolled for 7.x. Ran some of the node module tests as well as a quick manual test.
Also maybe we should consider fixing the mis-named "module" attribute of a node type back to "base" or to "hook" or something else less confusing.
#9
patch still applies (with offset)
#10
All tests pass with the patch (5765 passes, 0 fails, 0 exceptions)
#11
So I agree that the code that's working around this is utter cruft and should be done away with. However, I don't think
'module' => 'node_content'is a good fix, because there is no module called node_content, so this is going to be terribly confusing from a DX perspective.Since 'module' is a misnomer and is actually not meant to represent a module at all, but rather intended to be "the prefix applied to all node hook function names," a better solution is to rename that to something more descriptive while we're fixing this bug, and fix two problems at once. An informal poll of #drupal said sun's suggestion of 'hook_prefix' was best, and that seems the most intuitive to me.
#12
subscribing for later review
#13
The D6 schema description is wrong and misleading, so here's a patch we can come back to for that.
#14
changed as per webchick above. All tests still pass (5765 passes, 0 fails, 0 exceptions)
#15
after chx argued strongly against 'hook_prefix', I find his argument persuasive that these are not even hooks we are calling (hook meaning a per-module callback), I have re-rolled with just 'prefix' as a replacement for 'module'.
Also, updated the 2 affected node.module queries to use DBTNG.
Ran all tests - all pass.
#16
'prefix' is fine with me. Ran all tests with the patch applied and they pass fine.
#17
Sorry, really minor, but wrong indentation here:
+ $ret[] = update_sql("UPDATE {node_type} SET module = 'node_content' WHERE module = 'node'");+ db_change_field($ret, 'node_type', 'module', 'prefix', array('type' => 'varchar', 'length' => 255, 'not null' => TRUE));
Set back to RTBC afterwards.
#18
fixed indentation. Slight tweak to hook_schema wording.
#19
Hm. I am getting a bunch of these:
notice: Undefined property: stdClass::$prefix in /Applications/MAMP/htdocs/core/modules/node/node.module on line 626.
on admin/build/testing. Can anyone else confirm?
#20
Well, certainly helps when I run update.php. ;)
However, at the last step of update.php I get:
( ! ) PDOException: UPDATE {node_type} SET type=:db_update_placeholder_0, name=:db_update_placeholder_1, prefix=:db_update_placeholder_2, has_title=:db_update_placeholder_3, title_label=:db_update_placeholder_4, has_body=:db_update_placeholder_5, body_label=:db_update_placeholder_6, description=:db_update_placeholder_7, help=:db_update_placeholder_8, min_word_count=:db_update_placeholder_9, custom=:db_update_placeholder_10, modified=:db_update_placeholder_11, locked=:db_update_placeholder_12 WHERE (type = :db_condition_placeholder_501) - Array ( [:db_update_placeholder_0] => blog [:db_update_placeholder_1] => Blog entry [:db_update_placeholder_2] => blog [:db_update_placeholder_3] => 1 [:db_update_placeholder_4] => Title [:db_update_placeholder_5] => 1 [:db_update_placeholder_6] => Body [:db_update_placeholder_7] => A blog entry is a single post to an online journal, or blog. [:db_update_placeholder_8] => [:db_update_placeholder_9] => 0 [:db_update_placeholder_10] => 0 [:db_update_placeholder_11] => [:db_update_placeholder_12] => 1 [:db_condition_placeholder_501] => blog ) SQLSTATE[HY000]: General error: 1366 Incorrect integer value: '' for column 'modified' at row 1 in /Applications/MAMP/htdocs/core/includes/database/database.inc on line 365
Looks like this still needs work.
#21
for me, all tests pass ran all again) and the update runs fine when I apply the patch to a clean install.
#22
The problem came from having blog module installed, and node_type_save() which happens at the end of update.php when the cache is cleared. modified deafaults to a NULL value which is getting cast to 0. Peter is working on a fix.
#23
ah, the bug exhibits when blog module is enabled, not just default profile.
$info->modifieddoes not have a default value, so apparently DBTNG chokes trying to cast NULL to int. This adds a simple check along the lines of what's there for other values:+ if (empty($info->modified)) {+ $info->modified = 0;
+ }
#24
The ginormous insert and update queries in node_type_save() are a textbook case where you want to use a merge query instead, methinks. At minimum you want to build $fields in advance and just pass it in to each query, so you only have to maintain that array once, but I'm pretty sure that a merge query is the correct solution here.
#25
@Crell - do merge queries work? Are they documented? Why are you expanding the scope of the patch? A DBTNG conversion of those queries is really scope creep anyhow, so we should make it work, but refinement can come later. Also, we do not necessarily want to update all the fields - i.e. orig_type - when the type is edited. So, new patch this re-uses a fields array, but otherwise keeps the 2 separate queries.
Also, I think this fixes a bug with the insert query - we should insert $info->type for the orig_type field.
That aspect probably needs a D6 backport.
#26
Merge queries absolutely work according to the unit tests. Documentation is in progress. You also don't have to update all fields if you don't want to. (Or if you do, that's a bug.)
Whether or not merging those queries into one merge statement is scope creep or not I will defer to webchick.
#27
per IRC w/ webchick. prefix -> base. All tests pass, 1 exception unrelated to this patch (5801 passes, 0 fails, 1 exception)
#28
Base works just as well, and slightly less of a loaded term in core. So this should be ready to go. Already ran tests with prefix, ran some again with base, no issues (and I can confirm the exception is unrelated to this patch).
Documentation for merge queries is still a stub - so let's defer that to a later issue.
#29
Hm. Dang it!
( ! ) PDOException: INSERT INTO {node_type} (type, name, base, has_title, title_label, has_body, body_label, description, help, min_word_count, custom, modified, locked, orig_type) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9, :db_insert_placeholder_10, :db_insert_placeholder_11, :db_insert_placeholder_12, :db_insert_placeholder_13) - Array ( [:db_insert_placeholder_0] => poll [:db_insert_placeholder_1] => Poll [:db_insert_placeholder_2] => poll [:db_insert_placeholder_3] => 1 [:db_insert_placeholder_4] => Question [:db_insert_placeholder_5] => [:db_insert_placeholder_6] => [:db_insert_placeholder_7] => A poll is a question with a set of possible responses. A poll, once created, automatically provides a simple running count of the number of votes received for each response. [:db_insert_placeholder_8] => [:db_insert_placeholder_9] => 0 [:db_insert_placeholder_10] => 0 [:db_insert_placeholder_11] => 0 [:db_insert_placeholder_12] => 1 [:db_insert_placeholder_13] => poll ) SQLSTATE[HY000]: General error: 1366 Incorrect integer value: '' for column 'has_body' at row 1 in /Applications/MAMP/htdocs/core/includes/database/database.inc on line 365
Trying without the patch...
#30
Hm. Does not happen without this patch. Sorry, Peter. :(
I get this error when going to admin/build/modules and enabling all modules.
Without this patch I still get notices related to the registry, but nothing like this.
#31
@webchick - hey, no problem. This looks like a variant of the bug before - somebody is calling this function without setting 'has body' on the type.
#32
per IRC w/ webchick and Crell. DBTNG does not cast (e.g. bool to int) so for mysql it tends to choke and die if you pass a non-int to an int column. Also, we should make sure everything is set, so we don't pass a NULL where int is expected.
#33
With this patch I get "Call to undefined function function _node_type_set_defaults()" when I enable book module.
#34
yep, missed that one. Sorry.
diff to prior patch is just in book.install
Ran all tests with this patch: 5803 passes, 0 fails, 0 exceptions
#35
I can't run all tests at the moment due to Fatal error: Class 'DatabaseLog' not found in /home/catch/www/7.x/includes/database/database.inc on line 769
Which fails in HEAD too.
however all the tests up to that one passed, and enabling every module on admin/build/modules worked fine. Since I incorrectly marked this rtbc last time, leaving open for one more double check.
#36
patch still applies cleanly, all tests pass (5804 passes, 0 fails, 0 exceptions)
#37
omitting whitespace changes
#38
re-roll to eliminate extra function.
#39
Ok, this addresses all of my concerns. :)
#40
Docs needed for this momentous occasion! http://drupal.org/node/224333
#41
D'oh! Thanks Crell! I'm slackin'! :D
#42
draft doc done: http://drupal.org/node/224333#node_type_base
The correction to the schema description should be backported to 6.x
#43
backport of just the string change.
#44
#45
Thanks for the docs fix in D6. Committed.
#46
Automatically closed -- issue fixed for two weeks with no activity.