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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

matt2000’s picture

reporting that patch applies cleanly

swentel’s picture

FileSize
2.71 KB

Simple reroll, previous applied with some offsets (don't credit me)

cburschka’s picture

Patch looks good code-style and comment-wise - if Testbot gives it the go, mark it RTBC.

beeradb’s picture

Status: Needs review » Reviewed & tested by the community
beeradb’s picture

TestBot likes it, so does Arancaytar. There was much rejoicing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

lilou’s picture

Status: Needs work » Needs review
lilou’s picture

Status: Needs review » Reviewed & tested by the community

Back to #4

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
2.72 KB

Chasing HEAD.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Marking rtbc, see #3, #4 and #8

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed to HEAD. Thanks!

moshe weitzman’s picture

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

webchick’s picture

@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. :)

moshe weitzman’s picture

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.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

sun’s picture

Title: node_types_rebuild should rebuild menu » Regression: node_types_rebuild should rebuild menu
Status: Closed (fixed) » Active

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.

catch’s picture

Status: Active » Needs review
FileSize
2.61 KB

Here's a straight revert, then we can continue over at #193366: execute various rebuilds when modules are submitted for the performance issue itself.

EvanDonovan’s picture

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 :)

catch’s picture

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.

webchick’s picture

I'll try and look at this tomorrow.

sun’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Sorry, thought I had reverted this awhile ago. Committed to HEAD.

EvanDonovan’s picture

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.)

webchick’s picture

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.

EvanDonovan’s picture

Ok, great. I only asked b/c of catch's comments in #19.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.