Remove router rebuild from menu_ui.install

Hi,
drupal_flush_all_caches() moved from ModulesListForm::submitForm() to ModuleHandler::install()

Tested patch #3 and Drupal installation is finished without errors.

I missed the

include_once DRUPAL_ROOT . '/core/includes/menu.inc';

part.

3: base_system-2181151-3.patch queued for re-testing.

I think this is the correct approach. It loads stuff like path.inc the same way as in _drupal_bootstrap_code().

Borrowing from _menu_navigation_links_rebuild, which checks for menu_link first.

3: base_system-2181151-3.patch queued for re-testing.

Is this issue still relevant?

It seams that the core is changed a lot since this issue was active.

ModuleHandler::install() method does not longer exist, and ModulesListForm::submitForm() doesn't contain drupal_flush_all_caches() anymore.

Yep we're working to actively avoid flushing caches during module install. Re-titling the issue to focus on what we should be looking to do.

Let's simply give it a different try.

Let's see.

Ehem absolutely right!

Needs work for that...

Maybe something like this?

There we go.

Wondering if the added line in standard_install() could be considered an API change for install profiles, what happens exactly if you don't have that?

Also could be argued that we're possibly breaking contrib tests too although highly unlikely, and afaik we want to avoid that for 8.0.x as much as possible.

So, setting to RTBC for 8.1.x to get feedback on whether we are allowed to do this.

Also could be argued that we're possibly breaking contrib tests too although highly unlikely, and afaik we want to avoid that for 8.0.x as much as possible.

Is that statement actually true? I always thought that we don't support our tests as any form of API.

We don't support tests as any kind of API, but also we'd try to avoid breaking tests in patch releases so that people don't have to update their tests every month. I just committed #1494670: References to CSS, JS, and similar files should be root-relative URLs: avoids mixed content warnings & fewer bytes to send which breaks contrib tests, but in that case the tests were specifically checking for the buggy behaviour we were fixing.

Patch looks fine for 8.1.x though.

Do you know why we're flushing all caches in uninstall but not in install? I'm wondering if we need a follow-up to add that in.

Do you know why we're flushing all caches in uninstall but not in install? I'm wondering if we need a follow-up to add that in.

I don't see the difference. Both install and uninstall calls out to \Drupal::service('router.builder')->setRebuildNeeded();
Just the menu_ui module (which was some behaviour inherited from some of the renames over time) directly rebuilt it.

OK got it. Committed/pushed to 8.1.x.

Since there's theoretical breakage, not moving back to 8.0.x, but if this is actually affecting a site somehow please re-open.

Since there's theoretical breakage, not moving back to 8.0.x, but if this is actually affecting a site somehow please re-open.

For sites, the router is rebuild at the end of the request / installer (in theory), so if there is breakage than in tests. But yeah I don't care about 8.0.x here.