Inherits beta evaluation from parent issue: #2367749: Remove usage of deprecated UrlGeneratorInterface::generateFromPath()
Problem/Motivation
UrlGeneratorInterface::generateFromPath()
is deprecated for removal before Drupal 8.0.0 release.
All remaining usages in core/ should be removed. Those in system module were already handled in #2558189: Remove usage of deprecated UrlGeneratorInterface::generateFromPath() in system module
The only 2 pieces of non-test code that rely on this method are in the classes:
\Drupal\action\Plugin\Action\GotoAction
\Drupal\Core\EventSubscriber\RedirectResponseSubscriber
Those are both pretty trivial to remove. Everything else is just making tests happy.
Proposed resolution
Fix by changing the method call to generateFromRoute()
with a route name and parameters, or using Url::fromUserInput() or Url::fromUri()
All the things to be fixed:
core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
123: // @todo As generateFromPath() is deprecated, remove this in
132: $destination = $this->urlGenerator->generateFromPath($path, $options);
core/lib/Drupal/Core/Menu/menu.api.php
452: * to either \Drupal\Core\Routing\UrlGenerator::generateFromPath() or
469: * @see \Drupal\Core\Routing\UrlGenerator::generateFromPath()
core/lib/Drupal/Core/PathProcessor/OutboundPathProcessorInterface.php
25: * generateFromPath() method.
core/lib/Drupal/Core/Render/MetadataBubblingUrlGenerator.php
120: public function generateFromPath($path = NULL, $options = array(), $collect_bubbleable_metadata = FALSE) {
121: $generated_url = $this->urlGenerator->generateFromPath($path, $options, TRUE);
core/lib/Drupal/Core/Routing/UrlGenerator.php
375: public function generateFromPath($path = NULL, $options = array(), $collect_bubbleable_metadata = FALSE) {
core/lib/Drupal/Core/Routing/UrlGeneratorInterface.php
95: public function generateFromPath($path = NULL, $options = array(), $collect_bubbleable_metadata = FALSE);
core/lib/Drupal/Core/Utility/UnroutedUrlAssembler.php
181: // mirrors code from UrlGenerator::generateFromPath().
core/modules/action/src/Plugin/Action/GotoAction.php
79: ->generateFromPath($this->configuration['url'], array('absolute' => TRUE));
core/modules/help/tests/modules/help_test/src/SupernovaGenerator.php
42: public function generateFromPath($path = NULL, $options = array(), $collect_bubbleable_metadata = FALSE) {
core/modules/simpletest/src/BrowserTestBase.php
407: $url = $this->container->get('url_generator')->generateFromPath($path, $options);
core/modules/simpletest/src/WebTestBase.php
2691: $url = $this->container->get('url_generator')->generateFromPath($path, $options);
2954: return $this->container->get('url_generator')->generateFromPath($path, $options);
core/modules/views/src/Tests/Handler/FieldWebTest.php
265: $expected_result = \Drupal::urlGenerator()->generateFromPath('node/123', array('query' => array('foo' => NULL), 'fragment' => 'bar', 'absolute' => $absolute));
Remaining tasks
Final review/commit
User interface changes
n/a
API changes
n/a
Task | Novice task? | Contributor instructions | Complete? |
---|
Comment | File | Size | Author |
---|---|---|---|
#67 | 2575869-increment.txt | 1.92 KB | pwolanin |
#67 | 2575869-66.patch | 45.91 KB | pwolanin |
#62 | 2575869-increment.txt | 18.11 KB | pwolanin |
#62 | 2575869-62.patch | 45.33 KB | pwolanin |
#58 | 2575869-increment.txt | 1.48 KB | pwolanin |
Comments
Comment #2
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedMost things except WebTestBase. Let's see if these pass.
Comment #3
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedComment #6
JeroenTThis should fix a lot of the failing tests.
Comment #8
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedThanks, I think a couple places I forgot toString() but I got hung up trying to fix RedirectResponseSubscriberTest and didn't post the easy fix.
Comment #10
Wim LeersComment #11
dawehner@Wim Leers
Why should we not be able to replace more of those usages?
Comment #12
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedsome more tweaks on the way, let's see how the bot feels
Comment #13
Wim Leers#11: IIRC from the chat with xjm and pwolanin: because otherwise it'll be deprecated to D9, instead of deprecated sooner.
Comment #15
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedFix RedirectResponseSubscriber to use an injected unrouted URL assembler
Comment #16
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedAlmost there - let's see what still fails.
Comment #18
Wim LeersThis fixes the first failure.
Comment #20
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedapparently Url::fromUserInput() doesn't handle language prefixes (I think we knew that) so trying just base:
Comment #22
Wim LeersIn reviewing, I only found nits, that I quickly fixed myself:
s/url/URL/
Wrong FQCN.
This sounds a bit strange.
Comment #23
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedSpecial case
<front>
since it's not actually a pathComment #27
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedSimple fix for several fails - some of the absolute URLs also have added options.
Comment #28
dawehnerMh, its crazy that we have to do that. Can we at least add a follow up to maybe that such kind of logic accessible via the Url class?
Do we really need those changes?
Comment #31
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedWow this problem is like a nightmare that keeps recurring in Drupal\toolbar\Tests\ToolbarAdminMenuTest
Comment #32
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented@dawehner - yeah, seems like we don't really support
<front>
well, but neither did we remove it.Also, since unrouted URLs using base don't handle the outbound path processor to add a langcode, we need to use routes when a langcode option is specified it seems.
Comment #40
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedSome more test fixes.
Comment #43
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedUsing
internal:/
instead ofbase:
actually does what we want I think with many fewer changes needed.Let's see if that causes any other failure.
Comment #46
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedoops, that's causing lots of other fails. Let's just go back to the almost-passing version plus small fixes.
Here the increment is relative to #40
Comment #48
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedComment #50
Wim LeersComment #53
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedoops forgot to put back one last fix.
Comment #54
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedThe GotoAction should actually just work the same as the redirect code - using the unrouted assembler so we deliver just what the user typed in.
Coupling in the Url class also beings a lot of implicit dependencies.
Comment #56
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedUh, wow - need more sleep. I made the patch as the interdiff.
Comment #58
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedA couple small cleanups.
Comment #59
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedComment #61
Wim LeersNote this.
It is converted to using
base:
, which is converted to an URL by the code inUnroutedUrlAssembler::buildLocalUrl()
.But that code does this WRT outbound path processing:
And that option today is only set in … the unit test for
UnroutedUrlAssembler::buildLocalUrl()
.Therefore either the code changes or these (unchanged) docs are wrong when this patch is applied.
(And to confirm that these docs used to be correct:
UrlGenerator::generateFromPath()
, which was the code used here before, used to always apply path processing, as long as$options['external']
wasn'tTRUE
.)I think the fact that this had to be changed is further indication/confirmation of my previous point (because
WebTestBase::buildUrl()
was changed in a similar way); if path processing were enabled this would not have been necessary?And again here, and in other places.
Eh… this looks very wrong. :P
Comment #62
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedOk, per Wim Leers, trying to use the path processing option to avoid most of the test changes.
Comment #63
Wim LeersYay! No more integration test changes! This patch looks much better, and is basically only removing
generateFromPath()
now. Which makes the disruption effectively zero.IMO this is RTBC if it comes back green.
Comment #64
Wim LeersWatching the test runner at https://dispatcher.drupalci.org/job/default/21233/consoleFull, after about 65% of all tests have run, still not a single failure. RTBC'ing per #63.
Comment #67
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedThose 2 fails are coupled since
\Drupal\simpletest\Tests\SimpleTestBrowserTest::testTestingThroughUI
invokes\Drupal\Tests\simpletest\Functional\BrowserTestBaseTest
Trivial fix just to use slashes consistently in BrowserTestBaseTest
What's not obvious from the interdiff is that there are calls to drupalGet() in BrowserTestBase like
$this->drupalGet('user/logout')
that do not have a leading slash on the path parameter.Comment #68
YesCT CreditAttribution: YesCT commentedsince @Wim Leers was ok with rtbc'ing if it was green, and it is... I feel ok with an rtbc.
I talked to @pwolanin about why the fails happened and looked at the interdiff.
... because the path processor OutboundPathProcessorInterface requires a leading slash in the path.
[edit:
I also asked about
which seems like the opposite of what the interdiff with the test was doing.
But it is because in the test it is dealing with drupalGet
and in GotoAction, we are dealing with user input (where paths start with /)
this patch is not making everything consistent in that regard, because that was an existing thing, and it is trying to make as few changes as possible.
Comment #69
xjmNotes from my code review:
This looks correct and safe to me. UnroutedUrlAssembler::asseble() requires either an external (for external links) or
base:
, and we are enforcing the latter.These changes are because the hook is invoked from
LinkGenerator::generate()
. Which is still doing some weird stuff with paths, and also usinggetInternalPath()
which was "wishfully deprecated". @pwolanin found #2413701: Url::getInternalPath() is marked deprecated but won't be removed for 8.0.x, so update the docs. Anyway, that method usesUrl::toString()
which uses the unrouted URL assembler which doesn't usegenerateFromPath()
in HEAD anyway, so these docs were just wrong to begin with.I didn't know why this hunk was being moved. @pwolanin said it's because the path processor requires the leading slash, and so this is fixing a bug. Normally I'd suggest fixing said bug first in a separate issue, but given the special circumstances, let's file a followup for adding test coverage for said bug.
This is kind of hideous, but not as hideous as leaving the method around, I think. @pwolanin also suggested that we should add a followup for test coverage for actions.
@alexpott expressed some concern about needing to opt in for path processing, but I don't think this patch is the cause of that, and it does appear to be all for supporting the web test functionality.
er what? shouldn't these be the other way around? because we require a leading slash now?
I asked @pwolanin why this was and he said it's because BrowserTestBase seems to be agnostic about whether it gets a leading slash; some assertions had them and others didn't. The changes above make it consistent with WebTestBase.
I'd suggest a followup (it may already exist) to expect a leading slash instead, since we expect that in D8 in general, but that would require massive test changes so not needed now.
Looking over the CRs and such next.
Comment #70
larowlanInstead of changing the ->drupalGet calls, could we just use
ltrim($path, '/')
in drupalGet methodComment #71
xjmLet's also address #70 in a quick followup. The disruption should be low for now and I think it's much more important to get this in. We also should have followups for #69 points 3, 4, and six, and maybe there is an existing issue for point 5 but I don't know anything about it.
@YesCT and @pwolanin looked at the existing CRs and updated them for this change, and @pwolanin also posted https://www.drupal.org/node/2581455, which I have now published.
Committed and pushed to 8.0.x. Thanks everyone!
Comment #73
JeroenTCreated follow up for #70: #2581557: Add ltrim($path, '/') in drupalGet method
Comment #74
Wim LeersThanks, Jeroen!
Comment #75
JeroenTCreated the following issues:
Comment #76
JeroenTAll concerns mentioned by @xjm in #71 now have a separate issue, so removing the needs followup tag.