Problem/Motivation

#2372507-76: Remove _system_path from $request->attributes asked for a replacement for drupal_get_destination()

Proposed resolution

Add a 'redirect.destination' service with the following two methods:

  1. getAsArray() which is the same as drupal_get_destination()
  2. get() which just returns the path.

Remaining tasks

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task, because we want to able to allow proper testability of our code.
Issue priority Normal, because the impact is not that high
Disruption No disruption, because the BC layer is kept
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Title: Modernice drupal_get_destination() » Modernize drupal_get_destination()

.

dawehner’s picture

Issue summary: View changes
dawehner’s picture

Status: Active » Needs review
FileSize
4.73 KB

So what about this?

Status: Needs review » Needs work

The last submitted patch, 3: 2426805-3.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.73 KB
583 bytes

Good try :p

Status: Needs review » Needs work

The last submitted patch, 5: 2426805-5.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.38 KB
4.02 KB

Let's fix them, hopefully.

Status: Needs review » Needs work

The last submitted patch, 7: 2426805-7.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.41 KB
382 bytes

You also have to return a destination ...

Status: Needs review » Needs work

The last submitted patch, 9: 2426805-9.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.42 KB
756 bytes

Ups, wrong method.

dawehner’s picture

FileSize
10.57 KB
3.16 KB

Let's write a unit test.

cosmicdreams’s picture

Status: Needs review » Reviewed & tested by the community

That looks amazing. +1

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/includes/common.inc
    @@ -222,25 +222,7 @@ function drupal_get_html_head($render = TRUE) {
    +  return \Drupal::destination()->getDestinationArray();
    

    This is the only place where \Drupal::destination() is used.

    I don't see why we want to add yet another method on \Drupal if we can just as well use \Drupal::service('redirect.destination')?

  2. +++ b/core/lib/Drupal.php
    @@ -659,4 +659,14 @@ public static function accessManager() {
    +  /**
    +   * Returns the redirect destination helper.
    +   *
    +   * @return \Drupal\Core\Routing\RedirectDestination
    +   *   The redirect destination helper.
    +   */
    +  public static function destination() {
    +    return static::$container->get('redirect.destination');
    +  }
    

    Then this could go away.

  3. +++ b/core/lib/Drupal/Core/Routing/RedirectDestination.php
    @@ -0,0 +1,86 @@
    +   * The url generator.
    

    s/url/URL/

  4. +++ b/core/lib/Drupal/Core/Routing/RedirectDestination.php
    @@ -0,0 +1,86 @@
    +   * @param \Drupal\Core\Routing\UrlGeneratorInterface $url_generator
    +   */
    

    Incomplete doc — sorry :/

  5. +++ b/core/lib/Drupal/Core/Routing/RedirectDestination.php
    @@ -0,0 +1,86 @@
    +   * previous request, that destination is returned. As such, a destination can
    

    s/previous/current/

  6. +++ b/core/lib/Drupal/Core/Routing/RedirectDestination.php
    @@ -0,0 +1,86 @@
    +   * @ingroup form_api
    

    Are we sure about this?

  7. +++ b/core/lib/Drupal/Core/Routing/RedirectDestination.php
    @@ -0,0 +1,86 @@
    +   * Gets the destination as string.
    

    s/string/URL/
    or
    s/string/URL string/

  8. +++ b/core/modules/menu_ui/src/Plugin/Menu/LocalAction/MenuLinkAdd.php
    @@ -17,12 +20,52 @@
    -    $options['query']['destination'] = drupal_get_destination()['destination'];
    +    $options['query']['destination'] = $this->redirectDestination->getDestination();
    

    Nice :)

  9. +++ b/core/tests/Drupal/Tests/Core/Routing/RedirectDestinationTest.php
    @@ -0,0 +1,106 @@
    +   * The mocked url generator.
    

    s/url/URL/

dawehner’s picture

Status: Needs work » Needs review
FileSize
10.66 KB
2.66 KB

This is the only place where \Drupal::destination() is used.

I don't see why we want to add yet another method on \Drupal if we can just as well use \Drupal::service('redirect.destination')?

I think drupal_get_destination() is so commonly used that its fair to add a convenient wrapper.

Incomplete doc — sorry :/

I still think that we should update our coding standards to be less verbose by default, but this is probably a fight you can just loose.

Are we sure about this?

Meh, it used to be.

Wim Leers’s picture

I think drupal_get_destination() is so commonly used that its fair to add a convenient wrapper.

D'oh, of course. I thought this patch updated all occurrences, which is why I came to that conclusion.

Which point me to a pesky question: do we want to update all callers in this patch? There are 47 occurrences, some of which are in comments.

I still think that we should update our coding standards to be less verbose by default, but this is probably a fight you can just loose.

I'd be +1000000000 for that. We have way too many too silly documentation rules.

dawehner’s picture

Which point me to a pesky question: do we want to update all callers in this patch? There are 47 occurrences, some of which are in comments.

You know why we should not do that, and every instance of the past, was wrong? The patch size grew so much, that people don't concentrate on reviewing the actual functionality but rather get distracted by all the conversions at the same time.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

That's fair, but the patch size here is small and the actual changes are RTBC. But, no matter, let's do this!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

This issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

dawehner’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

Alright.

ParisLiakos’s picture

No interface?
Also RedirectDestination->getDestination() seems to verbose for me.
Why not just RedirectDestination->get() and getAsArray() ?
Finally shouldnt the service name be redirect_destination (underscore instead of dot)?

ParisLiakos’s picture

Status: Reviewed & tested by the community » Needs work

nw at least for interface

dawehner’s picture

Title: Modernize drupal_get_destination() » Remove 404 validation from link creation
Component: base system » link.module
Priority: Normal » Major
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update +D8 Accelerate NJ, +Needs reroll
Related issues: +#2417793: Allow entity: URIs to be entered in link fields

Boolean logic is hard to get right on the first time.

dawehner’s picture

Title: Remove 404 validation from link creation » Modernize drupal_get_destination()
Component: link.module » base system
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -D8 Accelerate NJ, -Needs reroll
Related issues: -#2417793: Allow entity: URIs to be entered in link fields

wow

Wim Leers’s picture

:D

dawehner’s picture

@ParisLiakos
Do you really think an interface is helpful?

ParisLiakos’s picture

an interface is always better to typehint with. if i want to swap this service with a class with different logic why would you force me to extend RedirectDestination?

dawehner’s picture

Status: Needs work » Needs review
FileSize
11.2 KB
6.34 KB

Well yeah, let's not fight about that.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

thanks :)

kgoel’s picture

Let's nitpick a bit :)
use Drupal\Core\Url;
While you are at it, remove unused class from MenuLinkAdd?

use Drupal\Component\Serialization\Yaml;
use Drupal\Component\Serialization\Exception\InvalidDataTypeException;
use Drupal\Component\Utility\Crypt;
use Drupal\Component\Utility\Number;
use Drupal\Component\Utility\Tags;
use Drupal\Core\Asset\AttachedAssets;
use Drupal\Core\Language\LanguageInterface;
use Drupal\Core\Site\Settings;
use Drupal\Core\Url;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpFoundation\Request;
use Drupal\Component\Utility\NestedArray;
use Drupal\Core\Datetime\DrupalDateTime;
use Drupal\Core\Routing\GeneratorNotInitializedException;

Lots of unused class in common.inc, won't hurt to remove them?

dawehner’s picture

FileSize
11.2 KB
568 bytes

Let's nitpick a bit :)
use Drupal\Core\Url;
While you are at it, remove unused class from MenuLinkAdd?

Fair

Lots of unused class in common.inc, won't hurt to remove them?

Feel free to open up a dedicated issue for that, ... I think this would be out of scope, and just makes the patch harder to understand.

kgoel’s picture

Feel free to open up a dedicated issue for that, ... I think this would be out of scope, and just makes the patch harder to understand.

Since issue was rtbc, it wouldn't have hurt to remove the unused classes and an interdiff would have make it clear.

willzyx’s picture

+++ b/core/lib/Drupal.php
@@ -667,4 +667,14 @@ public static function accessManager() {
+  /**
+   * Returns the redirect destination helper.
+   *
+   * @return \Drupal\Core\Routing\RedirectDestination
+   *   The redirect destination helper.
+   */
+  public static function destination() {
+    return static::$container->get('redirect.destination');
+  }

should be static::getContainer()->get('redirect.destination');

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

@willzyx - nice catch! Needs work for #33.

Also the beta evaluation needs a bit more work as to why we should do this during this phase of the release cycle.

dawehner’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
11.22 KB
605 bytes

Sure, there we go.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

dawehner’s picture

As it turns out, views resets the internal destination in an ajax request. Should we introduce a set function here,
or do that as part of #2412695: Remove drupal_static from ViewAjaxController::ajaxView ?

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/lib/Drupal/Core/Routing/RedirectDestination.php
@@ -0,0 +1,69 @@
+    if (!isset($this->destination)) {

protected $destination is missing.

What about test coverage for calling get() twice in a row that only runs the code once?

Also I don't see why we wouldn't do #37 here.

dawehner’s picture

FileSize
12.84 KB
3.52 KB

Alright, let's do it!

dawehner’s picture

FileSize
15.55 KB
2.71 KB

OH I see we have to fix views here, technically.

Status: Needs review » Needs work

The last submitted patch, 40: 2426805-40.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
17.14 KB
1.59 KB

dawehner--

tim.plunkett’s picture

  1. +++ b/core/tests/Drupal/Tests/Core/Routing/RedirectDestinationTest.php
    @@ -0,0 +1,140 @@
    +    $this->urlGenerator->expects($this->any())
    ...
    +    // Call in twice in order to ensure it returns the same the next time.
    +    $this->assertEquals($expected_destination, $this->redirectDestination->get());
    +    $this->assertEquals($expected_destination, $this->redirectDestination->get());
    ...
    +    // Call in twice in order to ensure it returns the same the next time.
    +    $this->assertEquals(['destination' => $expected_destination], $this->redirectDestination->getAsArray());
    +    $this->assertEquals(['destination' => $expected_destination], $this->redirectDestination->getAsArray());
    

    Hm, because it's $this->any(), we're not really asserting anything by calling this twice. Not sure if it's worth it.

  2. +++ b/core/tests/Drupal/Tests/Core/Routing/RedirectDestinationTest.php
    @@ -0,0 +1,140 @@
    +   * @dataProvider providerGet
    +   * @covers ::get
    

    extreeeeeeme nitpick, don't bother unless you reroll: Missing blank line.

  3. +++ b/core/tests/Drupal/Tests/Core/Routing/RedirectDestinationTest.php
    @@ -0,0 +1,140 @@
    +   * @dataProvider providerGet
    ...
    +  public function testSetBeforeGetCall() {
    ...
    +   * @dataProvider providerGet
    ...
    +  public function testSetAfterGetCall() {
    

    You don't have any params in these, so the data provider is meaningless, right?

Otherwise I think this is RTBC.

dawehner’s picture

FileSize
17.07 KB
831 bytes

Maybe someone else has an opinion regarding one, I personally dislike coupling the test to the internals, if possible.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

I think now that we have a set(), and the corresponding test coverage, it accomplishes what I had in mind anyway.
Awesome work @dawehner!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

Can we get a followup to remove drupal_get_destination from Drupal\views\Plugin\views\field\EntityOperations and Drupal\views\Tests\Plugin\views\field\EntityOperationsUnitTest

Also can the issue summary be updated with the actual solution instead of ideas?

dawehner’s picture

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update

We don't normally inline @todos for removing deprecated usages, so just opening the issue was enough.
@dawehner also updated the issue summary in #47.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Sorry - I missed the fact that this does not deprecate drupal_get_destination() for Drupal 9. Can we add that?

martin107’s picture

FileSize
17.31 KB
582 bytes

Added text,

Also a very minor nit-pick while reading the annotation associated with this change. The coding standard requires a newline between tags like @see and @ingroup

martin107’s picture

Status: Needs work » Needs review
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 08584b2 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 08584b2 on 8.0.x
    Issue #2426805 by dawehner, martin107: Modernize drupal_get_destination...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.