Regression: node_types_rebuild should rebuild menu
beeradb - September 10, 2008 - 03:54
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | node system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed |
Description
Ran into this issue when programmatically creating node types for book.test. Reposting my synopsis here:
After calling drupalWebTestCase::drupalCreateContentType() I have to do a menu_rebuild() to go to 'node/type/$type'. This rebuild is typically handled by node_type_form_submit which is never called in this context.
There are three options here...
- The menu_rebuild should be moved to node_types_rebuild.
- A menu_rebuild should be added to drupalCreateContentType()
- The menu_rebuild should be the tests responsibility to invoke, in this context
After a chat with chx in irc, he indicated that the menu_rebuild should be moved to node_types_rebuild, and all instances of node_types_rebuild and menu_rebuild called together should be updated. This patch does that.
marking critical, since the critical issue #299132: TestingParty08: book node types now relies on this patch.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| node_types_rebuild.patch | 2.57 KB | Idle | Failed: Failed to apply patch. | View details | Re-test |
#1
reporting that patch applies cleanly
#2
Simple reroll, previous applied with some offsets (don't credit me)
#3
Patch looks good code-style and comment-wise - if Testbot gives it the go, mark it RTBC.
#4
#5
TestBot likes it, so does Arancaytar. There was much rejoicing.
#6
The last submitted patch failed testing.
#7
See: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476
#8
Back to #4
#9
The last submitted patch failed testing.
#10
Chasing HEAD.
#11
Marking rtbc, see #3, #4 and #8
#12
Committed to HEAD. Thanks!
#13
I'm not so sure this is a good idea. When programmatically creating, you might create a few at a time and there is no reason to inflict multiple menu_rebuild() in this case. A programmer can handle some cache flush and menu rebuild himself. My .02
#14
@moshe: I don't think that's a problem, because node_type_save() doesn't called node_types_rebuild(). So people doing practical mass-importing of content types as part of a data import script or something will still have to rebuild the cache manually, which I think is fine for the reasons you outline.
However, when creating SimpleTests, clearing the cache ends up being one extra thing you need to remember when writing tests, so it's nice to put in a shortcut for these people when using drupalCreateContentType(). While it's possible a test creates 50 content types before doing its thing, I don't think that's the norm.
And personally, even when I am programmatically creating content types, I find that I always remember the node_types_rebuild() but end up forgetting the menu_rebuild() which puts the types under "Create content" like it ought to, so coupling them is a nice DX improvement, IMO.
That said, if you still think this is a bad idea, let's discuss it. :)
#15
thanks webchick. makes sense.
the core problem is that menu_rebuild() is brutally expensive. i'll pester the menu folks to give that some attention.
#16
Automatically closed -- issue fixed for 2 weeks with no activity.
#17
That, we have to revert. Menu system and Node system are two independent systems. If you want to, we can merge node types into the menu system or the menu system into node types. But as of now, these systems work and act separately.
See also http://api.drupal.org/api/function/menu_execute_active_handler/7 for an alternative solution.
#19
Here's a straight revert, then we can continue over at #193366: execute various rebuilds when modules are submitted for the performance issue itself.
#20
Thanks for this. I don't understand the technical details of what's happening here, but I hope the performance outcome will be beneficial.
Oh, and subscribing :)
#21
This won't have a big performance impact, but it decouples the menu_rebuild() from the node_types_rebuid() so we can use menu_rebuild() more selectively elsewhere.
#22
I'll try and look at this tomorrow.
#23
#24
Sorry, thought I had reverted this awhile ago. Committed to HEAD.
#25
Thanks!. Does this still have relevance to #193366: execute various rebuilds when modules are submitted, which has been RTBC for D6 since sun reviewed it last month? I would love it if that patch can make it into the next release of D6, as well as D7, of course. (I have been using #193366: execute various rebuilds when modules are submitted in production for months on a D6 site with 100+ modules and no known ill effects.)
#26
I don't think it has any bearing. The other patch simply moves the rebuilding to the submit button of the modules page rather than the display of it.
#27
Ok, great. I only asked b/c of catch's comments in #19.
#28
Automatically closed -- issue fixed for 2 weeks with no activity.