After calling node_type_save(), _node_types_build() doesn't bother to unset 'is_new', which creates bugs futher down the line. The new type is in the database, but node_type_get_types() says it isn't by virtue of is_new=1.
There is also currently a problem with static caching in _node_types_build() -- $_node_types is only linked with drupal_static() if (!$rebuild), meaning that during a rebuild, the static cache is not touched. Subsequent calls to node_type_get_types() may fall back on stale static data, and actions performed on $_node_types during node_types_rebuild() may not be active until a new page load.
This bug is blocking #890128: Comment body missing if comment module enabled after a content type module, so I'm marking critical.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | 993992-6-node-types-build.patch | 2.21 KB | carlos8f |
| #4 | 993992-4-node-types-build.patch | 2.8 KB | carlos8f |
| #3 | 993992-2-node-types-build-NO-STATIC-FIX.patch | 1.82 KB | carlos8f |
| #3 | 993992-2-node-types-build.patch | 2.68 KB | carlos8f |
| #1 | 993992-1-node-types-build-NO-STATIC-FIX.patch | 1.77 KB | carlos8f |
Comments
Comment #1
carlos8f commentedPatch which engages the static cache on rebuilds as well, and uses an internal 'init' property to determine if the static cache has been set or not.
Attached 2 patches to illustrate the static problem.
Comment #3
carlos8f commentedThese should apply :)
Comment #4
carlos8f commentedOh, of course resetting the static on $rebuild would be great, too!
Comment #6
carlos8f commentedOK, it's clear that fixing that dysfunctional cache_set()/static relationship is a bigger problem, here we simply invalidate the static cache on a rebuild.
Comment #7
dries commentedMmm. This is semantics not? If the node type is created during the current page request, I expect 'is_new' to be TRUE, even after it was saved. The behavior you want seems to be 'is_saved'.
Comment #8
carlos8f commentedAccording to the documentation of _node_types_build,
So it seems that this flag is supposed to mean that the type is not saved yet, and exists in hook_node_info() only. That's important to know since queries will fail on the {node_type} table.
The usage of this flag in comment_install() may be misguided, I'm still trying to determine that. But attempting to unset the flag made me realize that _node_types_build() fails to reset its static cache, which could cause a lot of problems in itself.
Comment #9
carlos8f commentedI've found a solution to #890128: Comment body missing if comment module enabled after a content type module that doesn't require this, so lowering priority appropriately. The static cache bug here could still cause headaches for contrib though.
Comment #10
David_Rothstein commentedI think this makes sense, based on the documentation.
Also, the first unset() in the patch definitely seems like it's heading in the right direction. Without that, content types like poll and blog which are defined in hook_node_info() always have $info->is_new set to TRUE, even when they are 8 months old or thereabouts :) However, I am wondering if there are times when we shouldn't unset that. Can there ever be a case where the info from hook_node_info() changes and therefore the database needs to be updated? I forget how this all works, what overrides what in that situation...
I'm also not sure I understand the test because with $this->resetAll() being called doesn't that mean that all caches are being cleared anyway so you're not actually testing the caching behavior of the function itself?
Comment #11
sunis_new may be used by modules to trigger dependent actions/inserts for new content types though.
Comment #12
tpopham commentedI'm working toward a better spec (and implementation) for _node_type_build() over at #1268974: For disabled node types with 'base' != 'node_content', _node_types_build() does not return data about the type.. I'm more interested in testing precise logic against the existing codebase. Since _node_types_build() is private to the node module, I'd like to make it generic, useful, and filter its behavior at the public methods.
I changed the spec of is_new to
I'm confused by
If you're dependent on a type implemented under hook_node_info(), then you should call node_types_rebuild(), right? (I noticed such use in the forum module). Anyway, it seems to me the solution to this bug is fixing the spec.
Thanks for the pointer to the caching problem, btw.
Comment #13
kscheirer#6: 993992-6-node-types-build.patch queued for re-testing.
Comment #15
mgiffordComment #24
quietone commented_node_types_build() was removed in Jun 2013, see #111715: Convert node/content types into configuration.
Closing as outdated. If that is wrong, reopen the issue, by setting the status to 'Active', and add a comment.