Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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:
getAsArray()
which is the same as drupal_get_destination()get()
which just returns the path.
Remaining tasks
User interface changes
API changes
Beta phase evaluation
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 |
Comment | File | Size | Author |
---|---|---|---|
#50 | interdiff-44-50.txt | 582 bytes | martin107 |
#50 | 2426805-50.patch | 17.31 KB | martin107 |
#44 | interdiff.txt | 831 bytes | dawehner |
#44 | 2426805-44.patch | 17.07 KB | dawehner |
#42 | interdiff.txt | 1.59 KB | dawehner |
Comments
Comment #1
dawehner.
Comment #2
dawehnerComment #3
dawehnerSo what about this?
Comment #5
dawehnerGood try :p
Comment #7
dawehnerLet's fix them, hopefully.
Comment #9
dawehnerYou also have to return a destination ...
Comment #11
dawehnerUps, wrong method.
Comment #12
dawehnerLet's write a unit test.
Comment #13
cosmicdreams CreditAttribution: cosmicdreams commentedThat looks amazing. +1
Comment #14
Wim LeersThis 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')
?Then this could go away.
s/url/URL/
Incomplete doc — sorry :/
s/previous/current/
Are we sure about this?
s/string/URL/
or
s/string/URL string/
Nice :)
s/url/URL/
Comment #15
dawehnerI think drupal_get_destination() is so commonly used that its fair to add a convenient wrapper.
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.
Meh, it used to be.
Comment #16
Wim LeersD'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'd be +1000000000 for that. We have way too many too silly documentation rules.
Comment #17
dawehnerYou 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.
Comment #18
Wim LeersThat's fair, but the patch size here is small and the actual changes are RTBC. But, no matter, let's do this!
Comment #19
alexpottThis 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.
Comment #20
dawehnerAlright.
Comment #21
ParisLiakos CreditAttribution: ParisLiakos commentedNo 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)?
Comment #22
ParisLiakos CreditAttribution: ParisLiakos commentednw at least for interface
Comment #23
dawehnerBoolean logic is hard to get right on the first time.
Comment #24
dawehnerwow
Comment #25
Wim Leers:D
Comment #26
dawehner@ParisLiakos
Do you really think an interface is helpful?
Comment #27
ParisLiakos CreditAttribution: ParisLiakos commentedan 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?
Comment #28
dawehnerWell yeah, let's not fight about that.
Comment #29
ParisLiakos CreditAttribution: ParisLiakos commentedthanks :)
Comment #30
kgoel CreditAttribution: kgoel commentedLet's nitpick a bit :)
use Drupal\Core\Url;
While you are at it, remove unused class from MenuLinkAdd?
Lots of unused class in common.inc, won't hurt to remove them?
Comment #31
dawehnerFair
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.
Comment #32
kgoel CreditAttribution: kgoel commentedSince issue was rtbc, it wouldn't have hurt to remove the unused classes and an interdiff would have make it clear.
Comment #33
willzyx CreditAttribution: willzyx commentedshould be static::getContainer()->get('redirect.destination');
Comment #34
alexpott@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.
Comment #35
dawehnerSure, there we go.
Comment #36
dawehnerBack to RTBC
Comment #37
dawehnerAs 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 ?
Comment #38
tim.plunkettprotected $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.
Comment #39
dawehnerAlright, let's do it!
Comment #40
dawehnerOH I see we have to fix views here, technically.
Comment #42
dawehnerdawehner--
Comment #43
tim.plunkettHm, because it's $this->any(), we're not really asserting anything by calling this twice. Not sure if it's worth it.
extreeeeeeme nitpick, don't bother unless you reroll: Missing blank line.
You don't have any params in these, so the data provider is meaningless, right?
Otherwise I think this is RTBC.
Comment #44
dawehnerMaybe someone else has an opinion regarding one, I personally dislike coupling the test to the internals, if possible.
Comment #45
tim.plunkettI think now that we have a set(), and the corresponding test coverage, it accomplishes what I had in mind anyway.
Awesome work @dawehner!
Comment #46
alexpottCan we get a followup to remove drupal_get_destination from
Drupal\views\Plugin\views\field\EntityOperations
andDrupal\views\Tests\Plugin\views\field\EntityOperationsUnitTest
Also can the issue summary be updated with the actual solution instead of ideas?
Comment #47
dawehnerHere it is one, for all of them.
Comment #48
tim.plunkettWe don't normally inline @todos for removing deprecated usages, so just opening the issue was enough.
@dawehner also updated the issue summary in #47.
Comment #49
alexpottSorry - I missed the fact that this does not deprecate drupal_get_destination() for Drupal 9. Can we add that?
Comment #50
martin107 CreditAttribution: martin107 commentedAdded 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
Comment #51
martin107 CreditAttribution: martin107 commentedComment #52
dawehnerThank you!
Comment #53
alexpottCommitted 08584b2 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.