Access denied after install - menu_rebuild() calls

I'm not sure now, but I think there might be some change in the permissions, and update from betas is not supported (don't ask me why... no clue...), so you should probably try with a fresh 6.0-rc1 install (or 6.x-dev better), to see whether this is really a bug. (BTW assign yourself, only if you're going to work on the fix)

I can't reproduce this so assuming it's an upgrade from beta issue.

I looked into the code, and can't see any big changes since beta4 there, apart from 'edit any forum topic' permission added. I would suggest double-checking the permissions (any new ones there, not assigned yet?), submit them, and hit the central cache-clearing button on the Performance settings page (may not hurt...)

Aero - can you try a fresh install of rc-1 or -dev, and see if you still get the issue? So we know it's definitely an upgrade bug.

If it is only an upgrade bug, then that's not supported. Although I'd suggest disabling and uninstalling the forum module, then re-enabling to start off with.

I am raising priority for this, I tried installing fresh D6 rc1 and got Access denied on many pages, including "Configure" at Garland theme. Further investigation shows, that I have no Access rules on the site.

I wondered if this was caused by locale module which I was testing, but after installing fresh RC1 again, without locale module, everything is the same.

Conditions:
D6 RC1
MySQLi
No locale module
USED table prefix

It requires to rebuild the menu after installation, then it works -> for garland configure page

Sorry, didn't meant access rules, this has nothing to do with them. But the bug still appears.

This reveals quite broad problem: Stale data in menu_router after install. (Yes, I reproduced the Access denied on Garland config page)

- First, I think that system_menu() should force refresh on themes_list() - system.module line 295 should be: foreach (list_themes(TRUE) as $theme) { - note the added TRUE argument.

- But that's not enough. Since menu_rebuild() is done at the very end of install.php, it fixes various data from that stage in menu_router (because various modules return dynamic data in their hook_menu() implementations, including system.module responsible for the theme settings pages). I think we should use drupal_flush_all_caches() at the end of install.php for consistency, and move menu_rebuild() to the very very end of that function to ensure all hook_menu() implementations return their data based on fresh input.

- But there's more: themes_list() behave differently under maintenance mode, does no database reads, and returns just a list of existing themes. All with status=0 (disabled)! This then gets stored into menu_router (still under maintenance mode at the end of istall.php), and here we are: Garland is in fact disabled (for menu access).

- I can't see clean solution right now:
* Unfortunately, we can't cancel the maintenance mode before final menu_rebuild() inside install.php, because it's indicated by a constant being defined, and there's no way to un-define a constant.
* Returning all themes with status=1 from themes_list() would be wrong again (all themes enabled, as menu goes),
* hardcoding 'garland' name is also wrong (the same menu_rebuild() under maintenance happens in update.php too, where the set of actually enabled themes may be different)

- The last bullet indicates, that the problem with access to Garland settings denied might also happen after update.php run (untested)

- We probably need more general solution, like getting menu_rebuild() out of maintenance mode. Any data elsewhere in Drupal might have the same problem.

- So perhaps the only solution would be to set a flag only inside drupal_flush_all_caches() - assuming that we use it in install.php too - and postpone the actual menu_rebuild() run to the next page request. (Perhaps trigger menu_rebuild() just by the fact of menu cache being empty?) That would be consistent with other caches handling, and should solve the maintenance mode problems (and menu_rebuild() need of being last).

There's quite a bit to discuss here, it seems...

Cross-posted, sorry... Fixing title.

@jirkarybka There is a duplicate of this issue http://drupal.org/node/202938

I did a small patch for garland theme access
http://drupal.org/node/202938#comment-667706

But it might not be a very good sollution.

The patch file in that other issue is empty. But checking against default theme, as suggested in there, will not help for other active themes (after update.php).

TRUNCATE is support by mysql, pgsql, mssql, oracle.

@chx: TRUNCATE TABLE {menu_router} is more save, isn't it? TABLE is mandatory for MsSQL... as an example.

mysql> truncate cache;
Query OK, 0 rows affected (0.07 sec)

http://www.postgresql.org/docs/7.4/interactive/sql-truncate.html

TRUNCATE [ TABLE ] name

TABLE is mandatory for MsSQL - it's better to be compatible. All others like pgswl, oracle use the same syntax with table. We HOPEFULLY get MsSQL support with D7... so it's better to be same for now.

I am not fighting this past RC1 but note that I will fight any patches that do anything solely on the ground of "MSSQL support".

Works after patching...

Now what about update.php? Seems like the same problem is going to happen there, too. Also I would rather adjust drupal_clear_all_caches() to this style too, just to be safe, as it's likely to be used in a lot of various cases in the future (I proposed it for update.php myself on another issue).

We can surely do this.

Simpler version based on if (defined('MAINTENANCE_MODE')).

(Previous patch was result on PM conversation).

So this looks better, as it acknowledges the difference between the live pages and maintenance pages, and how they can rebuild the menu. But we need code comments to that effect on why is this done and that menu_execute_active_handler() will do the rebuild if this menu rebuild call is invoked on maintenance pages.

More doxygen.

Wow! This is quite nice - simple, and makes menu_rebuild() really bullet-proof, whatever we do elsewhere.

But I'm a bit uneasy about adding yet another query to every single page request just for the empty router check... I think we should rather variable_set('menu_rebuild_postponed', TRUE), and variable_del() then again, so that the per-page check doesn't need extra query, while extra variable is stored only rarely.

CNW so that we can try and get rid of that query.

Well, that query runs extremely fast on MySQL because it reads table info not the table itself. But surely we can do a variable here and yes it's even faster.

I'm not sure I like the proposed fix -- I think it's a hack. Using a variable for communication like this is considered to be bad practice. Running an extra SQL query on every page view is not desirable either. Furthermore, the proposed change makes it hard to understand what is going on inside menu_rebuild() and the PHPdoc fails short explaining this. Let's keep looking for a more acceptable solution ...

That might be not too easy.

themes_list() behave differently under maintenance mode, does no database reads, and returns just a list of existing themes.

there can be any number of functions that show different behaviour on this maintenance mode so a successful menu_rebuild in maintenance mode seems out of questions. Now, if it needs to run once we get out of maintenance mode, then it needs to be communicated. Note that there is no explicit set of maintenance mode so I can't simply run a rebuild in some submit handler. I could set SESSION but that's quite error prone. I could truncate and check after menu_get_ancestors come back empty handed but that's a much more expensive solution than the one proposed here. I am asking you to reconsider.

subscribe ... i have a DB that fails during upgrade to D6 because of memory exhaustion during upgrade in menu rebuild. i will see if this patch helps. but it will take me a while so i just subscribe for now.

I managed to reproduce the access denied bug with garland settings page after update. Ran a full 5.x-6.x upgrade with and without the patch, and it's fixed by #27. I got an undefined index notice after update.php but leaving at needs review, since that's irrelevant to whether this approach is fine or not.

Subscribing (and confirming again the access denied on garland page).

OK with this patch I'm also getting a notice on install (makes sense).

And I noticed that in my Navigation menu, at the top level I have

# Navigation
# Primary links
# Secondary links

each of which links off the relevant: /admin/build/menu-customize/menu-name page

So back to needs work.

chx: your docs on the install.php / update.php case are indeed inaccurate:

+ * ... If called from update.php or install.php, it will only
+ * truncate the {menu_router} table and menu_execute_active_handler will call
+ * this function on the first real page load.

Subscribing.

Those links are coming from the menu_enable at it presents quite a big problem: it saves the menu links which do not yet have their parents (which come from menu_menu). We need to record those links to somewhere for reparenting after the menu_rebuild. I guess a variable will do -- could even be the same variable, just store an array in there. I will patch at a later time.

pwolanin (not for the first time) gave us a fantastic idea: rebuild the router no matter what and flag for an additional rebuild. This keeps the links table in order and fixes up the router.

Better doxygen.

This is minor, but if you have to re-roll again, this comment should end in a period:

+ * menu_execute_active_handler,

Tested and works (at least for the Garland link) w/ PHP 4.4.7.

Tested this with a fresh install and it also works fine for me on the Garland settings page (not aware of anywhere else to check the 403). Also checked modules admin page and that was fine too. This is on PHP 5.2.

So seems ready to me. Quick edit version of the patch with that period instead of the comma, obviously don't credit me if it's committed (or I guess it could be tidied on commit from #39 anyway, but just in case).

Updated the docs to say the following, explaining the reason to rerun menu_rebuild() after maintenance mode:

/**
 * (Re)populate the database tables used by various menu functions.
 *
 * This function will clear and populate the {menu_router} table, add entries
 * to {menu_links} for new router items, then remove stale items from
 * {menu_links}. If called from update.php or install.php, it will also
 * schedule a call to itself on the first real page load from
 * menu_execute_active_handler(), because the maintenance page environment
 * is different and leaves stale data in the menu tables. 
 */

Committed this one. Thanks for the patch/testing.