\Drupal::state()->set('menu_rebuild_needed') is not guaranteed to be checked on the next request [#2167323]

Updated: Comment #N

Problem/Motivation

Using \Drupal::state()->set('menu_rebuild_needed') indicates that the menu (router) needs rebuilding, but helps ensure it is only done once per request.
However, the code that checks for the flag and does the rebuilding is in menu_get_item(), which is being called less and less.
This means that the menu will only rebuilt on the next call to menu_get_item(), and you don't know when that will be.

Proposed resolution

Move that check and rebuilding to a listener, probably for KernelEvents::RESPONSE?

Remaining tasks

Decide on approach, write patch

User interface changes

N/A

API changes

N/A

Comment	File	Size	Author
#3	state-2167323-3.patch	3.18 KB	tim.plunkett
#3

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 5 January 2014 at 06:32

Parent issue:

» #2107533: Remove {menu_router}

Comment #2

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 5 January 2014 at 06:33

Title:

\Drupal::state()->set('menu_rebuild_needed') is not guaranteed to be called on the next request

» \Drupal::state()->set('menu_rebuild_needed') is not guaranteed to be checked on the next request

Comment #3

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 5 January 2014 at 08:21

Status:

Active

» Needs review

File	Size
state-2167323-3.patch	3.18 KB

Here is one approach. Works for failure I saw in another issue (#2042807: Convert search plugins to use a ConfigEntity and a PluginBag).

Comment #4

5 January 2014 at 09:11

Status:

Needs review

» Needs work

The last submitted patch, 3: state-2167323-3.patch, failed testing.

Comment #5

Crell CreditAttribution: Crell commented 5 January 2014 at 09:27

RESPONSE isn't a good place for this; that blocks the user response.

However, here's a possibility. Wherever it is that we trigger the need for a menu rebuild... don't set state at all. Set an anonymous function event listener for kernel::TERMINATE. That way it fires after the response is set, but in the same process. No need to wait for the next request, we simply punt it to the end of the current process. No state tracking needed.

We can probably put the actual logic into a service, and have the closure simply call that. That way we can still unit test it. I know we did something very similar elsewhere a while back, but I can't recall where. It was a ParisLiakos patch, I think...

Comment #6

catch

he/him

English

CreditAttribution: catch commented 5 January 2014 at 11:53

This was originally added by #202955: Access denied after install - menu_rebuild() calls to workaround calling menu_rebuild() in an incomplete environment during the installer. We have installer tests now, so moving it to the end of the request rather than the next request ought to maintain the performance improvement without regressing the bug. If it doesn't while the installer isn't much better than it was this was introduced, it ought to be more fixable in there.