Remove menu_execute_active_handler()

Other than the function itself I found more (even non-comment) references than I expected:

./core/includes/menu.inc: * menu_execute_active_handler() to generate your page output.
./core/includes/menu.inc:    // be available in menu_execute_active_handler() resulting in a 404.
./core/includes/common.inc: * bubble up to menu_execute_active_handler() should call drupal_site_offline().
./core/includes/common.inc: * bubble up to menu_execute_active_handler() should call drupal_not_found().
./core/includes/common.inc: * bubble up to menu_execute_active_handler() should call
./core/includes/common.inc: * This function is most commonly called by menu_execute_active_handler(), but
./core/includes/common.inc: * When a user requests a page, index.php calls menu_execute_active_handler(),
./core/includes/common.inc: * menu_execute_active_handler(), this function gets called. The purpose of
./core/includes/common.inc: *   function (e.g., menu_execute_active_handler()). If not given, it is
./core/includes/common.inc: * @see menu_execute_active_handler()
./core/includes/common.inc:          $return = menu_execute_active_handler($path, FALSE);
./core/includes/common.inc:          $return = menu_execute_active_handler($path, FALSE);
./core/modules/system/system.api.php: * menu_execute_active_handler(). If the site is in offline mode,
./core/modules/system/tests/http.php:menu_execute_active_handler();
./core/modules/system/tests/https.php:menu_execute_active_handler();
./core/modules/system/tests/menu.test:    // Now we enable the rebuild variable and trigger menu_execute_active_handler()
./core/modules/system/tests/menu.test:    // menu_execute_active_handler() should trigger the rebuild.
./core/modules/comment/comment.module:    return menu_execute_active_handler('node/' . $node->nid, FALSE);
./core/modules/user/user.pages.inc:    return menu_execute_active_handler(NULL, FALSE);

I'm a little confused with this request. I asked to Crell about this request and he said this function not need to be re-writed, because will be replaced by Kernel::handle($request).

But looks like the function was already updated to use some symfony2 stuff.

Can you double check this issue still requiere some work and explain a little better what is necessary or maybe close this issue.

Regards

enzo

See also #1622934-9: Replace delivery callbacks by leveraging the HTTP kernel, fix the overlay module .

Trickier than expected, because:

1. You need the kernel to do a sub request. Is there a different way than trying to access the global kernel variable? Left in @todo's where I did that.
2. The request() container functions needs to be updated with the new request, when doing subrequests.
3. We didn't convert http.php and https.php testing front controllers to the kernel model. They were still using the old flow. Changed that.

This is probably blocking the critical issue I referenced in #4.

Pager.inc

-  $page = isset($_GET['page']) ? $_GET['page'] : '';
+  $page = request()->get('page', '');

Should be

-  $page = isset($_GET['page']) ? $_GET['page'] : '';
+  $page = request()->query->get('page', '');

Oh ... indeed. Thanks.

Edit: I also should have mentioned that the only remaining references to menu_execute_active_handler() are in includes/common.inc doxygen -- on functions that already have issues for removal.

Putting some debugging stuff in there, because I can't reproduce it locally.

Crell in irc we concluded that the $request was empty because it was build way to late in bootstrap phase. I don't see why the DI container fixes this problem?

There are problems that prevent us from easily converting the pager to use requests() rather than $_GET. I'd vote for handling that in a seperate issue and getting this one done as soon as possible. So the attached patch is just a straight conversion from menu_execute_active_handler() to subrequests, still leaving in the $_GET['page'] = $page and a corresponding TODO.

I'd also vote for not blocking on #1595146: Load the HttpKernel from the DI Container. There's a TODO in the code and we can revisit that, once that patch is in.

+++ b/core/modules/user/user.pages.incundefined
@@ -492,10 +494,12 @@ function user_cancel_confirm($account, $timestamp = 0, $hashed_pass = '') {
+    // @todo: Get the kernel from a container, cleaner sub request handling.

Isn't this TODO precise enough?

Yay, we can now avoid adding new globals using #1595146: Load the HttpKernel from the DI Container! Rerolled the patch (that also no longer applied) and did that.

Forgot some stuff :)

Hmm forgot to remove the use statements we don't need anymore

Yes, thanks, that looks better. Probably going to be green and good to go.

Removed the stammering.

The drupal_boostrap() is in index.php, too:

// Bootstrap all of Drupal's subsystems, but do not initialize anything that
// depends on the fully resolved Drupal path, because path resolution happens
// during the REQUEST event of the kernel.
// @see Drupal\Core\EventSubscriber\PathSubscriber;
// @see Drupal\Core\EventSubscriber\LegacyRequestSubscriber;
drupal_bootstrap(DRUPAL_BOOTSTRAP_CODE);

$kernel = drupal_container()->get('httpkernel');
// [...]

Not sure why, but we should mirror that in the tests.

Committed to 8.x. Thanks!

Wow. :)

It would be great if we could find a way to announce major changes like this. I understand that the intention is to collect/summarize all these changes in a single change notice. However, that inherently circumvents any API change notification mechanism (whereas the relaying to @drupal8changes is the most effective from my perspective). In other words, I'm fine with the idea of collecting/summarizing related changes, but would highly appreciate some kind of an announcement for actual API changes.

To facilitate that, we should make sure to at least tag issues accordingly with one or more of "API change", "API addition", or "API clean-up"

If all goes well, this comment will trigger a new change notification mechanism already. :)