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.
Comment | File | Size | Author |
---|---|---|---|
#19 | menu_rebuild_revert.patch | 2.61 KB | catch |
#10 | node_types_rebuild_0.patch | 2.72 KB | swentel |
#2 | node_types_rebuild.patch | 2.71 KB | swentel |
node_types_rebuild.patch | 2.57 KB | beeradb | |
Comments
Comment #1
matt2000 CreditAttribution: matt2000 commentedreporting that patch applies cleanly
Comment #2
swentel CreditAttribution: swentel commentedSimple reroll, previous applied with some offsets (don't credit me)
Comment #3
cburschkaPatch looks good code-style and comment-wise - if Testbot gives it the go, mark it RTBC.
Comment #4
beeradb CreditAttribution: beeradb commentedComment #5
beeradb CreditAttribution: beeradb commentedTestBot likes it, so does Arancaytar. There was much rejoicing.
Comment #7
lilou CreditAttribution: lilou commentedSee: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476
Comment #8
lilou CreditAttribution: lilou commentedBack to #4
Comment #10
swentel CreditAttribution: swentel commentedChasing HEAD.
Comment #11
swentel CreditAttribution: swentel commentedMarking rtbc, see #3, #4 and #8
Comment #12
webchickCommitted to HEAD. Thanks!
Comment #13
moshe weitzman CreditAttribution: moshe weitzman commentedI'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
Comment #14
webchick@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. :)
Comment #15
moshe weitzman CreditAttribution: moshe weitzman commentedthanks webchick. makes sense.
the core problem is that menu_rebuild() is brutally expensive. i'll pester the menu folks to give that some attention.
Comment #17
sunThat, 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.
Comment #19
catchHere's a straight revert, then we can continue over at #193366: execute various rebuilds when modules are submitted for the performance issue itself.
Comment #20
EvanDonovan CreditAttribution: EvanDonovan commentedThanks 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 :)
Comment #21
catchThis 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.
Comment #22
webchickI'll try and look at this tomorrow.
Comment #23
sunComment #24
webchickSorry, thought I had reverted this awhile ago. Committed to HEAD.
Comment #25
EvanDonovan CreditAttribution: EvanDonovan commentedThanks!. 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.)
Comment #26
webchickI 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.
Comment #27
EvanDonovan CreditAttribution: EvanDonovan commentedOk, great. I only asked b/c of catch's comments in #19.