Remove instances of menu_get_object('node')

+++ b/core/modules/node/node.module
@@ -621,8 +622,12 @@ function node_show(EntityInterface $node, $message = FALSE) {
+  if ($request->attributes->has('node')) {

You can't do this for node_is_page(), because on something like a comment page that has the node in the request context, this will be a false positive. In some of these places, you actually need to check the route name.

If you reread the "menu_get_object() is used for two main reasons:" from the parent/meta issue, this is #1

1. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument/Date.php
   @@ -42,16 +45,53 @@ class Date extends Formula {
   +    $form['default_argument_type']['#options'] += array('node_changed' => t("Current node's update time"));
   

   If we change this line anyway we should use $this->t() instead.

2. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument/Date.php
   @@ -62,18 +102,11 @@ public function getDefaultArgument($raw = FALSE) {
   -      foreach (range(1, 3) as $i) {
   -        $node = menu_get_object('node', $i);
   -        if (!empty($node)) {
   -          continue;
   -        }
   -      }
   -
   -      if (arg(0) == 'node' && is_numeric(arg(1))) {
   -        $node = node_load(arg(1));
   +      if ($this->request->attributes->has('node')) {
   +        $node = $this->request->attributes->get('node');
          }
   

   What a win if you compare the two ones.

Thank you

1. +++ b/core/modules/book/lib/Drupal/book/Plugin/Block/BookNavigationBlock.php
   @@ -63,7 +63,9 @@ public function blockSubmit($form, &$form_state) {
   +      $node = $request->attributes->get('node');
   

   This is quite ugly, and another good reason to do #2124749: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API, but it's not the fault of this patch.

2. +++ b/core/modules/node/lib/Drupal/node/Plugin/views/argument_default/Node.php
   @@ -10,6 +10,9 @@
     * Default argument plugin to extract a node via menu_get_object
   

   This reference should be removed.

Yes a function's comment shouldn't explain implementation issues like what functions it calls.

Also updated the patch to apply to the most recent version of the repository.

Tagging for reroll.

rerolled patch

Thank you for taking the time to inject things properly.

1. +++ b/core/modules/node/lib/Drupal/node/Plugin/views/argument_default/Node.php
   @@ -21,17 +24,51 @@
   +      if ($node instanceof NodeInterface) {
   

   What else would $node be?

2. +++ b/core/modules/node/node.module
   @@ -577,7 +579,10 @@ function node_revision_delete($revision_id) {
   +  if ($request->attributes->get(RouteObjectInterface::ROUTE_NAME) == 'node.view') {
   

   Not sure I like exposing the Symfony CMF RouteObjectInterface for such a common operation here.

1. see point 1 here https://drupal.org/comment/7937111#comment-7937111 :)
2. $request->attributes->get(RouteObjectInterface::ROUTE_NAME) is used in couple places
   block_page_build
   TermBreadcrumbBuilder::applies
   menu_link_system_breadcrumb_alter
   \Drupal\forum\ForumBreadcrumbBuilder
   and few more.
   And these are in core. Why is usage of RouteObjectInterface common in node.module?

Yeah technically the code previous did just checked whether the nid at position 1 in the path agrees with the nid passed in, so yeah I guess we can keep the same logic, too.

So needs work, let's just check whether the node object exists and confirms with the passed in one.

According to #2177041: Remove all implementations of hook_menu, that issue is blocked by this, which makes this a beta blocker?

+++ b/core/includes/theme.inc
@@ -2241,8 +2241,9 @@ function template_preprocess_page(&$variables) {
 
-  if ($node = menu_get_object()) {
-    $variables['node'] = $node;
+  $request = \Drupal::request();
+  if ($request->attributes->has('node')) {
+    $variables['node'] = $request->attributes->get('node');
   }

+++ b/core/modules/node/node.module
@@ -577,7 +579,10 @@ function node_revision_delete($revision_id) {
+  $request = \Drupal::request();
+  if ($request->attributes->get(RouteObjectInterface::ROUTE_NAME) == 'node.view') {
+    $page_node = $request->attributes->get('node');
+  }

Am I the only one that thinks the Request object has a weird API? :)

+++ b/core/includes/theme.inc
@@ -2241,8 +2241,9 @@ function template_preprocess_page(&$variables) {
 
-  if ($node = menu_get_object()) {
-    $variables['node'] = $node;
+  $request = \Drupal::request();
+  if ($request->attributes->has('node')) {
+    $variables['node'] = $request->attributes->get('node');
   }

Actually, get() returns NULL by default (you can even control what is returned by default), that would be much easier than the conditionals that we have here?

Instead of this, we just write:

if ($node = \Drupal::request()->attributes->get('node')) {
  ...
}

Which is almost as easy as before, unlike the current change?

Would also save some of the introduced !empty($node) checks, which I always find weird (having a defined variable and if ($node) is imho easier to read, what is an "empty" $node ;)).

Good ideas, seriously.

That looks much better I think :)

Those default argument plugins are pretty crazy, how they just fall back to the node fields and try ti find some term ID's in whatever way they can :)

embarrassing

31: menu_get_object-node-2095959-31.patch queued for re-testing.

+++ b/core/modules/node/node.module
@@ -1159,26 +1163,23 @@ function node_block_access($block) {
-      return FALSE;
+    // This is a node creation page.
+    // $request->attributes->has('node_type') would also match administration
+    // configuration pages, which the previous URI path options did not.
+    if ($request->attributes->get(RouteObjectInterface::ROUTE_NAME) == 'node.add') {
+      $node_type = $request->attributes->get('node_type');
+      return in_array($node_type->id(), $allowed_types);
     }

I'm not sure about having comments that reference previous code. It might make sense in the interdiff, but for someone looking at the resulting code (in a year, or two), it won't.

So maybe simply move the first comment line into the if or rewrite (Check if this is...) and then replace the second part of the second sentence with "so check specifically for the node.add route" ?

Ok, that's too complicated, I mean something like this:

// Only looking for node_type request parameter would also match
// administration configuration pages, specifically limit it to the node.add route.
if ($request->attributes->get(RouteObjectInterface::ROUTE_NAME) == 'node.add') {
  // This is a node creation page.
  $node_type = $request->attributes->get('node_type');
  return in_array($node_type->id(), $allowed_types);
}

And as ugly as that route name check is, I agree with #22 that it's already used in HEAD and shouldn't block this, but maybe we can open a followup issue? I think it's really unfortunate that we have to expose the fact that we are using Symfony/Cmf for the routing for a simple (?) check like this.

1. +++ b/core/modules/book/lib/Drupal/book/Plugin/Block/BookNavigationBlock.php
   @@ -18,7 +21,44 @@
   +   * Constructs a Drupal\Component\Plugin\PluginBase object.
   

   BookNavigationBlock

2. +++ b/core/modules/node/lib/Drupal/node/Plugin/views/argument_default/Node.php
   @@ -21,17 +24,49 @@
   +   * Constructs a default argument User object.
   

   Node object.

3. +++ b/core/modules/node/node.module
   @@ -590,7 +594,7 @@ function node_is_page(EntityInterface $node) {
   +  if (($node = \Drupal::request()->attributes->get('node')) && $node instanceof NodeInterface) {
   

   How come we check for instanceof NodeInterface some places and not others?

4. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/argument_default/Tid.php
   @@ -22,6 +25,42 @@
   +   * Constructs a default argument User object.
   

   Tid object.

5. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument/Date.php
   @@ -41,16 +44,53 @@ class Date extends Formula {
   +   * Constructs a default argument User object.
   

   Date object.

And as ugly as that route name check is, I agree with #22 that it's already used in HEAD and shouldn't block this, but maybe we can open a followup issue?

I'm not sure this is really the same as the existing usages.

menu_get_object() is a commonly used API in contrib/custom modules, and we're replacing it with something that is extremely ugly here. If we open a follow-up to fix that, it'll be two API changes instead of one (and that follow-up would probably need to be critical/beta-blocker). The current usages are at least mostly implementation details/crept in - i.e. I don't think we have this in a change notice yet.

menu_get_object() is a commonly used API in contrib/custom modules, and we're replacing it with something that is extremely ugly here. If we open a follow-up to fix that, it'll be two API changes instead of one (and that follow-up would probably need to be critical/beta-blocker). The current usages are at least mostly implementation details/crept in - i.e. I don't think we have this in a change notice yet.

This issue is blocking #2177041: Remove all implementations of hook_menu and so a full chain of related issues. Afaik there was an issue which did nothing else then provide a service which allows you to fetch the route name for example from a service instead from the request object.

How come we check for instanceof NodeInterface some places and not others?

This makes it both safer but also more suitable for static code analyze tools/IDEs.

We have #2124749: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API. That doesn't actually encompass these constants, but it could.

This is why I originally opened #2103301: Add a helper class to introspect a given request.

But I don't think we should hold this issue up on that, this is a huge blocker for the menu system changes.

Hm, I'm not sure #36 is addressed entirely. I can understand the desire to get this in and unblock the rest of the MenuSystemRevamp, but I'd want, at a minimum, an explicit beta blocker followup if we're not going to fix it here.

Edit: I missed that catch tagged #2124749-15: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API. On the fence as to whether that covers entirely though.

Let's do this.

Looks like we forgot two calls, one of them was actually converted below :)

Given that these were the last usages of that function, let's just drop it and close the meta?

Didn't mean to leave it at RTBC.

Good catch!

I'm OK doing this in the other issue - exactly the same lines of code need to be touched, even if it's not quite the same problem with exposing the Symfony CMF interface. Committed/pushed to 8.x, thanks!

Needs a change notice, we should explicitly state that the API might change when #2124749: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API is resolved though.

Thank you, here is a change notice: https://drupal.org/node/2192317

Slightly updated and published.