Problem/Motivation

current_path() is basically a wrapper around $request->attributes->get('_system_path')which is itself considered to be an internal API only

Proposed resolution

Convert the uses of current_path() to the appropriate alternative.
This involves:

  • Adding route support for $batch
  • Set the <none> route by default before routing got executed.
  • Add route support for #ajax
  • Add Url::fromRouteMatch()
  • Simplify JS apis
  • This requires having the route available in tests

Remaining tasks

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task,
before
Issue priority Critical because its not using the internal _system_path is a critical.
CommentFileSizeAuthor
#94 current_path-fixup.patch3.38 KBjbrown
#93 2362227-callback-93.patch1.58 KBolli
#92 book-ajax-full-form.png48.21 KBjbrown
#79 2362227-79.patch51.84 KBdawehner
#79 interdiff.txt1.79 KBdawehner
#72 2362227-current_path-72.patch51.95 KBtim.plunkett
#72 interdiff.txt1.24 KBtim.plunkett
#65 2362227-65.patch51.4 KBdawehner
#65 interdiff.txt2.44 KBdawehner
#64 interdiff-62-64.txt324 bytesmpdonadio
#64 replace_all_instances-2362227-64.patch49.16 KBmpdonadio
#62 interdiff-60-62.txt942 bytesmpdonadio
#62 interdiff-58-62.txt1.41 KBmpdonadio
#62 replace_all_instances-2362227-62.patch48.66 KBmpdonadio
#60 interdiff-58-60.txt596 bytesmpdonadio
#60 replace_all_instances-2362227-60.patch47.95 KBmpdonadio
#58 2362227-58.patch47.6 KBdawehner
#53 interdiff-50-53.txt506 bytesmpdonadio
#50 interdiff-41-50.txt4.41 KBmpdonadio
#50 2362227-50.patch48.56 KBmpdonadio
#48 interdiff.txt5.35 KBznerol
#44 log-only-do-not-test.patch3.46 KBznerol
#41 interdiff.txt8.11 KBdawehner
#41 2362227-41.patch51 KBdawehner
#39 interdiff.txt515 bytesdawehner
#39 2362227-39.patch49.61 KBdawehner
#35 interdiff.txt4.76 KBdawehner
#35 2362227-35.patch49.56 KBdawehner
#33 interdiff.txt4.01 KBdawehner
#33 2362227-33.patch45.54 KBdawehner
#31 interdiff.txt2.29 KBdawehner
#31 2362227-31.patch42.19 KBdawehner
#29 interdiff.txt10.79 KBdawehner
#29 2362227-29.patch43.08 KBdawehner
#26 2362227-26.patch45.94 KBdawehner
#26 interdiff.txt4.5 KBdawehner
#24 interdiff.txt4.6 KBdawehner
#24 2362227-22.patch43.74 KBdawehner
#22 interdiff.txt1.29 KBskipyT
#22 2362227-22.patch41.42 KBskipyT
#20 interdiff.txt3.73 KBdawehner
#20 2362227-20.patch41.37 KBdawehner
#18 2362227-18.patch40.91 KBdawehner
#18 interdiff.txt1.79 KBdawehner
#16 interdiff.txt4.48 KBdawehner
#16 2362227-16.patch40.79 KBdawehner
#10 interdiff.txt12.12 KBdawehner
#7 2362227-7.patch39.69 KBdawehner
#5 interdiff.txt5.63 KBdawehner
#5 2362227-5.patch27.62 KBdawehner
#1 2362227-1.patch28.02 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

FileSize
28.02 KB

This is kinda a rough start because we need to add support for routes in quite a lot of places still (if this does not break, the world is kinda broken).

dawehner’s picture

Status: Needs work » Needs review

.

Status: Needs review » Needs work

The last submitted patch, 1: 2362227-1.patch, failed testing.

Wim Leers’s picture

(if this does not break, the world is kinda broken)

:D

  1. +++ b/core/lib/Drupal/Core/Path/PathMatcher.php
    @@ -98,6 +109,7 @@ public function isFrontPage() {
    +      // @todo page.front should store the route name.
    

    That makes a lot of sense! (Well, route name and parameters, of course.)

  2. +++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
    @@ -145,7 +146,7 @@ public static function processAjaxForm(&$element, FormStateInterface $form_state
    -   *   - #ajax['path']
    +   *   - #ajax['url']
    

    This makes sense on the surface, but I don't yet see the full repercussions. (I don't think anybody is around anymore who *thoroughly* knows the AJAX system? If there is, we should ping that person.)

    I'm only wondering why Url objects are used — is it a conscious choice to not use route name + parameters?

  3. +++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
    @@ -248,8 +249,11 @@ public static function preRenderAjaxForm($element) {
    +      if (isset($settings['url']) && ($url = $settings['url']) && $url instanceof Url) {
    

    Why not just $settings['url'] instanceof Url
    ?

    And I think you also want to check if $settings['options'] is set?

  4. +++ b/core/lib/Drupal/Core/Routing/NullRouteMatch.php
    @@ -56,4 +57,10 @@ public function getRawParameters() {
    +  }
     }
    

    Missing newline.

  5. +++ b/core/modules/update/src/UpdateManager.php
    @@ -148,18 +148,23 @@ public function projectStorage($key) {
    +      // @todo find out the corresponding route for that.
    +      // case 'admin/appearance/update':
    

    It seems this simply doesn't exist anymore?

  6. +++ b/core/modules/views/views.theme.inc
    @@ -301,10 +301,12 @@ function template_preprocess_views_view_summary(&$variables) {
    +    // Count be an alias.
    

    s/Count/Could/

  7. +++ b/core/modules/views_ui/admin.inc
    @@ -322,20 +323,31 @@ function views_ui_standard_display_dropdown(&$form, FormStateInterface $form_sta
    + *   The URl object pointing to the form URL.
    

    s/URl/URL/

  8. +++ b/core/modules/views_ui/src/ViewUI.php
    @@ -756,7 +757,9 @@ public function renderPreview($display_id, $args = array()) {
    +    if ($request_stack->getCurrentRequest() != $current_request) {
    

    This needs documentation. It looks as if the Views rendering pipeline *might* have popped the request we added two hunks higher, but not necessarily?
    Please explain that here.

  9. +++ b/core/tests/Drupal/Tests/Core/Routing/RouteMatchTest.php
    @@ -55,6 +55,7 @@ public function testRouteMatchFromRequest() {
     
    +
         // A routed request with parameter upcasting.
    

    Unnecessary change.

dawehner’s picture

Status: Needs work » Needs review
FileSize
27.62 KB
5.63 KB

This makes sense on the surface, but I don't yet see the full repercussions. (I don't think anybody is around anymore who *thoroughly* knows the AJAX system? If there is, we should ping that person.)

I'm only wondering why Url objects are used — is it a conscious choice to not use route name + parameters?

We do use url objects in more places now that route_name/parameters.

Why not just $settings['url'] instanceof Url

... You don't review code with storm right?

And I think you also want to check if $settings['options'] is set?

One hunk above:

 $settings += array(
        'path' => isset($settings['callback']) ? 'system/ajax' : NULL,
        'options' => array(),
It seems this simply doesn't exist anymore?

No, it does exist, see #2362205: admin/theme/install|update should point to admin/appearance/update|install for more information about that madness.

This needs documentation. It looks as if the Views rendering pipeline *might* have popped the request we added two hunks higher, but not necessarily?
Please explain that here.

A bit more explanations here ...

Status: Needs review » Needs work

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
39.69 KB

More work, I think we should split this up later though.

Status: Needs review » Needs work

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

Wim Leers’s picture

... You don't review code with storm right?

No, why?

One hunk above:

Oops, sorry!

A bit more explanations here ...

Thanks!

#7: could you still post an interdiff, while you probably still have this commit in your git history?

dawehner’s picture

Status: Needs work » Needs review
FileSize
12.12 KB

Ups, sure.

catch’s picture

Component: base system » routing system
znerol’s picture

+++ b/core/lib/Drupal/Core/Routing/RouteMatch.php
@@ -140,6 +141,13 @@ public function getRawParameters() {
   /**
+   * {@inheritdoc}
+   */
+  public function getUrl() {
+    return Url::fromRoute($this->getRouteName(), $this->getRawParameters()->all());
+  }

You could achieve the same effect by introducing URL::createFromRouteMatch() instead. This would be cleaner because then you do not need to pollute the RouteMatch class with all sorts of implicit dependencies. See also #2296205: Create a \Drupal\Component\Routing\ namespace and move Drupal-independent routing classes into it (wants to move RouteMatch out of core namespace).

dawehner queued 7: 2362227-7.patch for re-testing.

Status: Needs review » Needs work

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
40.79 KB
4.48 KB

@znerol
I like you idea. Will implement that after sending it to the testbot this time. Many of these places though could be replaced by using
the '' route.

Status: Needs review » Needs work

The last submitted patch, 16: 2362227-16.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.79 KB
40.91 KB

Some debugging later.

Status: Needs review » Needs work

The last submitted patch, 18: 2362227-18.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
41.37 KB
3.73 KB

There we go.

Status: Needs review » Needs work

The last submitted patch, 20: 2362227-20.patch, failed testing.

skipyT’s picture

Status: Needs work » Needs review
FileSize
41.42 KB
1.29 KB

I tried to fix the failed tests for the shortcut module and the view preview.

Status: Needs review » Needs work

The last submitted patch, 22: 2362227-22.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
43.74 KB
4.6 KB

This could be it.

Status: Needs review » Needs work

The last submitted patch, 24: 2362227-22.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.5 KB
45.94 KB

There we go.

znerol’s picture

Great that this finally turned green. #13 still applies though.

tim.plunkett’s picture

+++ b/core/modules/shortcut/src/Tests/ShortcutLinksTest.php
@@ -203,7 +203,7 @@ public function testShortcutLinkDelete() {
-  public function testNoShortcutLink() {
+  public function ptestNoShortcutLink() {

Well we don't know if its green yet :)

dawehner’s picture

FileSize
43.08 KB
10.79 KB

Come on, you should trust me :P

tim.plunkett’s picture

  1. +++ b/core/includes/batch.inc
    @@ -451,18 +451,21 @@ function _batch_finished() {
    +      if (!$redirect instanceof \Drupal\Core\Url) {
    

    Drupal\Core\Url is already imported here.

  2. +++ b/core/includes/batch.inc
    @@ -451,18 +451,21 @@ function _batch_finished() {
    +        $options = UrlHelper::parse($redirect);
    ...
    +          $options = UrlHelper::parse($redirect);
    

    We're calling this twice and don't need to.

  3. +++ b/core/lib/Drupal/Core/Path/PathMatcher.php
    @@ -98,6 +111,7 @@ public function isFrontPage() {
    +      // @todo page.front should store the route name.
    

    Can we get an issue?

  4. +++ b/core/lib/Drupal/Core/Routing/NullGenerator.php
    @@ -38,6 +38,9 @@ protected function getRoute($name) {
    +    else if ($name == '<current>') {
    

    elseif

  5. +++ b/core/lib/Drupal/Core/Routing/NullRouteMatch.php
    @@ -7,6 +7,7 @@
    +use Drupal\Core\Url;
    
    +++ b/core/lib/Drupal/Core/Routing/RouteMatch.php
    @@ -7,6 +7,7 @@
    +use Drupal\Core\Url;
    

    Unused imports

  6. +++ b/core/lib/Drupal/Core/Url.php
    @@ -582,11 +606,11 @@ protected function unroutedUrlAssembler() {
    -   *   The URL generator.
    +   *   (optional) The URL generator, specify NULL to reset it.
    ...
    -  public function setUrlGenerator(UrlGeneratorInterface $url_generator) {
    +  public function setUrlGenerator(UrlGeneratorInterface $url_generator = NULL) {
         $this->urlGenerator = $url_generator;
         return $this;
    

    What happens if you specify NULL and then immediately call a method that uses it?

  7. +++ b/core/modules/views_ui/src/ViewUI.php
    @@ -757,7 +764,11 @@ public function renderPreview($display_id, $args = array()) {
    -    _current_path($old_q);
    +    // Ensure that we just remove an additional request we pushed earlier.
    +    // This could happen if $errors was not empty.
    +    if ($request_stack->getCurrentRequest() != $current_request) {
    +      $request_stack->pop();
    +    }
    

    Yay

dawehner’s picture

@tim.plunkett
This should have been marked as needs work :)

elseif

Oh shit, I'm so out of core :)

What happens if you specify NULL and then immediately call a method that uses it?

Well, in that case \Drupal::urlGenerator() is used again.

tim.plunkett’s picture

There are still 4 remaining calls to current_path():

  • drupal_get_destination()
  • _drupal_add_js()
  • system_page_attachments()
  • UserLoginBlock::build()
dawehner’s picture

FileSize
45.54 KB
4.01 KB

Let's give it a try and fix all the other instances.

Status: Needs review » Needs work

The last submitted patch, 33: 2362227-33.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
49.56 KB
4.76 KB

Maybe this is it

Status: Needs review » Needs work

The last submitted patch, 35: 2362227-35.patch, failed testing.

catch’s picture

jibran’s picture

Issue tags: +JavaScript
dawehner’s picture

Status: Needs work » Needs review
FileSize
49.61 KB
515 bytes

Its green!

znerol’s picture

  1. +++ b/core/includes/common.inc
    @@ -1473,11 +1474,14 @@ function _drupal_add_js($data = NULL, $options = NULL) {
    +          /** @var  $request */
    

    Missing type hint.

  2. +++ b/core/includes/form.inc
    @@ -839,7 +840,7 @@ function batch_process($redirect = NULL, $url = 'batch', $redirect_callback = NU
    -      'source_url' => current_path(),
    +      'source_url' => Url::fromRouteMatch(\Drupal::routeMatch()),
    

    $batch['source_url'] is changed from a plain string to a URL object, right? Seems legit. I have no idea what happens when an Url object is serialized, I guess it is okay?

  3. +++ b/core/lib/Drupal/Core/Path/PathMatcher.php
    @@ -84,7 +96,8 @@ public function matchPath($path, $patterns) {
    +      $this->isCurrentFrontPage = ($url->getRouteName() && $url->getInternalPath() == $this->getFrontPagePath());
    

    Not sure whether this is official policy now, but I guess we should use identity operator whenever possible (===).

  4. +++ b/core/lib/Drupal/Core/Path/PathMatcher.php
    @@ -98,6 +111,7 @@ public function isFrontPage() {
    +      // @todo page.front should store the route name.
    

    Link to #2371823: Frontpage should maybe store the route name.

  5. +++ b/core/lib/Drupal/Core/Routing/NullRouteMatch.php
    @@ -18,14 +19,14 @@ class NullRouteMatch implements RouteMatchInterface {
    -    return NULL;
    +    return '<none>';
       }
     
       /**
        * {@inheritdoc}
        */
       public function getRouteObject() {
    -    return NULL;
    +    return new Route('<none>');
    

    This also cropped up in the original issue #2238217-130: Introduce a RouteMatch class. Back then there was no <none> route though, so the change is probably ok. But maybe we need to adapt the @return documentation.

  6. +++ b/core/lib/Drupal/Core/Url.php
    @@ -582,11 +606,11 @@ protected function unroutedUrlAssembler() {
    -  public function setUrlGenerator(UrlGeneratorInterface $url_generator) {
    +  public function setUrlGenerator(UrlGeneratorInterface $url_generator = NULL) {
         $this->urlGenerator = $url_generator;
         return $this;
       }
    

    Not sure: Do we need to also reset the new $internalPath property here?

  7. +++ b/core/modules/simpletest/src/InstallerTestBase.php
    @@ -162,13 +162,6 @@ protected function setUp() {
    -    // When running from run-tests.sh we don't get an empty current path which
    -    // would indicate we're on the home page.
    -    $path = current_path();
    -    if (empty($path)) {
    -      _current_path('run-tests');
    -    }
    -
    
    +++ b/core/modules/simpletest/src/WebTestBase.php
    @@ -920,12 +920,6 @@ protected function setUp() {
    -    // Temporary fix so that when running from run-tests.sh we don't get an
    -    // empty current path which would indicate we're on the home page.
    -    $path = current_path();
    -    if (empty($path)) {
    -      _current_path('run-tests');
    -    }
    

    Nice!

  8. +++ b/core/modules/system/src/Tests/Theme/ThemeTest.php
    @@ -137,14 +140,17 @@ public function testThemeOnNonHtmlRequest() {
         // Set the current path to node because theme_get_suggestions() will query
    ...
    -    \Drupal::config('system.site')->set('page.front', 'node')->save();
    ...
    +    $request->attributes->set(RouteObjectInterface::ROUTE_OBJECT, new Route('/user/login'));
    

    Also update the comment.

  9. +++ b/core/modules/system/src/Tests/Theme/ThemeTest.php
    @@ -137,14 +140,17 @@ public function testThemeOnNonHtmlRequest() {
    +    \Drupal::service('current_route_match', NULL);
    

    This looks like a noop.

  10. +++ b/core/modules/system/tests/modules/system_test/system_test.module
    @@ -99,16 +99,16 @@ function system_test_lock_exit() {
    +  if ($route_name == 'system_test.main_content_fallback') {
    ...
    +  elseif ($route_name == 'system_test.main_content_handling') {
    

    Use identity operator.

  11. +++ b/core/modules/update/update.module
    @@ -120,25 +120,26 @@ function update_page_top() {
    +      // @todo find out the corresponding route for that.
    +      // case 'admin/appearance/update':
    

    It is update.theme_update and the path is /admin/theme/update (which should be fixed, but probably in another issue).

  12. +++ b/core/tests/Drupal/Tests/Core/Routing/CurrentRouteMatchTest.php
    @@ -52,13 +52,15 @@ public function testGetCurrentRouteObject() {
    +    $request_stack->push(clone $request);
    

    I don't get it why it is necessary to clone the request here. Perhaps add a comment.

  13. +++ b/core/tests/Drupal/Tests/Core/Routing/RouteMatchTest.php
    @@ -37,8 +38,8 @@ public function testRouteMatchFromRequest() {
    +    $this->assertEquals('<none>', $route_match->getRouteName());
    

    Use assertSame.

  14. +++ b/core/tests/Drupal/Tests/Core/Routing/RouteMatchTest.php
    @@ -63,6 +64,9 @@ public function testRouteMatchFromRequest() {
    +    $url = Url::fromRouteMatch($route_match);
    +    $this->assertSame('test_route', $url->getRouteName());
    +    $this->assertSame(['foo' => '1'] , $url->getRouteParameters());
    

    I think this should be moved to UrlTest unit test.

I skipped changes in JavaScript and Views for now.

dawehner’s picture

FileSize
51 KB
8.11 KB

Missing type hint.
/blockquote>
Ha! I probably just dropped writing it after realizing that \Drupal::request() already gives you the typehint information.

$batch['source_url'] is changed from a plain string to a URL object, right? Seems legit. I have no idea what happens when an Url object is serialized, I guess it is okay?

I would assume it does, given that

class Url {
  use DependencySerializationTrait;
Not sure whether this is official policy now, but I guess we should use identity operator whenever possible (===).

Sure ...

This also cropped up in the original issue #2238217-130: Introduce a RouteMatch class. Back then there was no route though, so the change is probably ok. But maybe we need to adapt the @return documentation.

Adapted

Not sure: Do we need to also reset the new $internalPath property here?

Sure ... done

This looks like a noop.

Sure, let's drop it.

It is update.theme_update and the path is /admin/theme/update (which should be fixed, but probably in another issue).

Yeah you are right ...

I don't get it why it is necessary to clone the request here. Perhaps add a comment.

CurrentRouteMatch uses $request as internal caching key ... altering the request does not internally updates cached key,
but cloning a request ensures that the following code works again:
$this->routeMatches[$request]

I think this should be moved to UrlTest unit test.

Sure, fixed it.

znerol’s picture

There is another issue related to #40.5. It seems to me that this change was necessary in order to make Url::fromRouteMatch() work when current route match returns a NullRouteMatch. This is most probably the case for code running before routing was performed.

I believe that in those cases we cannot simply replace current_path() with Url::fromRouteMatch(\Drupal::routeMatch())->getInternalPath() because that will produce different results.

Also git grep -- '->getRouteObject(' turned up some occurrences where the return value was used in a conditional, i.e. some code is only executed when getRouteObject returns something.

That said, I think that the changes to NullRouteMatch might probably do more harm than any good.

dawehner’s picture

There is another issue related to #40.5. It seems to me that this change was necessary in order to make Url::fromRouteMatch() work when current route match returns a NullRouteMatch. This is most probably the case for code running before routing was performed.

Well, this is also necessary for tests in which no routing as performed or the installer, where you don't have routing available.

That said, I think that the changes to NullRouteMatch might probably do more harm than any good.

Well, do you have any suggestion how to otherwise solve it? Should we change it to use new Route('') and implement the route
processor in a way that it does not use the route match but rather the _system_path property or figuring out that path
from the Request object, manually?

znerol’s picture

FileSize
3.46 KB

Let's first try to understand whether this is really a problem. Attached is a patch which bluntly adds some logging to core maintenance mode subscriber (running after routing), user maintenance mode subscriber (running before routing) and kernel pre-handle middleware (running before path alias is resolved).

Add node/1 with an alias an-alias-to-node-1. The results are as follows:

Scenario A: only log from core maintenance mode subscriber:

MaintenanceModeSubscriber (core, after routing)
request_path(): an-alias-to-node-1
current_path(): node/1
from route match (alias: FALSE): /an-alias-to-node-1
from route match (alias: TRUE): /node/1

Seems legit.

Scenario B: log before and after routing (i.e. from core and user maintenance mode subscriber):

MaintenanceModeSubscriber (user module, before routing)
request_path(): an-alias-to-node-1
current_path(): node/1
from route match (alias: FALSE): /
from route match (alias: TRUE): /
MaintenanceModeSubscriber (core, after routing)
request_path(): an-alias-to-node-1
current_path(): node/1
from route match (alias: FALSE): /
from route match (alias: TRUE): /

This is the most interesting test-case, which a) indicates that we really should not attempt to generate Urls from NullRouteMatch and b) exposes a static-cache problem in CurrentRouteMatch.

Scenario C: log at all three places:

KernelPreHandle (before alias is resolved)
request_path(): an-alias-to-node-1
current_path(): an-alias-to-node-1
from route match (alias: FALSE): /
from route match (alias: TRUE): /
MaintenanceModeSubscriber (user module, before routing)
request_path(): an-alias-to-node-1
current_path(): node/1
from route match (alias: FALSE): /
from route match (alias: TRUE): /
MaintenanceModeSubscriber (core, after routing)
request_path(): an-alias-to-node-1
current_path(): node/1
from route match (alias: FALSE): /
from route match (alias: TRUE): /

No surprise here, before resolving the alias current_path() == request_path().

Frankly I do not have much of an idea on how to resolve that. I think it is clear from the logs above that we cannot possibly use anything involving the route match before routing actually happened.

The first thing which should be determined is whether it is possible to only use the unaliased-path (i.e. request_path instead of current_path) outside of routed code (controllers). Those occurrences could maybe be substituted with pathinfo?

dawehner’s picture

@znerol

So would you agree that for these few rare usecases we actually still need something like $request_match->getSystemPath() or something similar on some other object?

znerol’s picture

$request_match?

dawehner’s picture

Ehem $route_match

znerol’s picture

FileSize
5.35 KB

Let's turn that around: Instead of trying to make route match capable of coping with the installer, make all the crap calling prepareLegacyRequest() behave.

The interdiff rolls back the modifications to route match and instead pretends that legacy requests are routed. Maybe it is also necessary to adapt WebTestBase::prepareRequestForGenerator(), not sure about that.

dawehner’s picture

@znerol

Do you want to file a patch and see whether the test fails?

mpdonadio’s picture

FileSize
48.56 KB
4.41 KB

I like making patches...

Status: Needs review » Needs work

The last submitted patch, 50: 2362227-50.patch, failed testing.

znerol’s picture

While manually testing I've stumbled upon another issue in both #41 and #50. It seems that the URLs generated in JavaScript are broken. E.g., the toolbar menu ajax callback fails and therefore icons are not displayed in the sidebar. Also subtrees are missing there.

This seems to be related to the changes in the Drupal.url function. There is no property baseUrl on drupalSettings.path. Is this maybe a leftover from an earlier patch?

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
506 bytes
+++ b/core/misc/drupal.js
@@ -326,7 +326,7 @@ if (window.jQuery) {
   Drupal.url = function (path) {
-    return drupalSettings.path.basePath + drupalSettings.path.scriptPath + drupalSettings.path.pathPrefix + path;
+    return drupalSettings.path.baseUrl + drupalSettings.path.pathPrefix + path;
   };
 

This is why the JS broke. _drupal_add_js() doesn't have the equivalent change. Not sure where the / came from in the old version; added it in _drupal_add_js() instead on the JS side. Manual testing `Drupal.url('foo/bar')` seems to work as intended (I am in a subdir locally).

Not seeing .basePath used anywhere.

dawehner’s picture

I still think that the NUllRouteMatch should return a NULL route ... just in general it is quite important that code like _drupal_add_js
does not fail when we are not in routing context, as for example exception pages should still work, if possible.

dawehner’s picture

Issue summary: View changes
dawehner’s picture

Issue summary: View changes
mgifford’s picture

Would be good to move this issue forward.

dawehner’s picture

Status: Needs work » Needs review
FileSize
47.6 KB

Rerolled for now and fixed the failures. Just to be clear, the statement in #54 is still important. We call code outside of the context of routing, for example on exception pages ...

Status: Needs review » Needs work

The last submitted patch, 58: 2362227-58.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
47.95 KB
596 bytes

Didn't into the test fail from #59, that that patch didn't address #52 (likely because I didn't attach the actual path, just the interdiff).

Status: Needs review » Needs work

The last submitted patch, 60: replace_all_instances-2362227-60.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
48.66 KB
1.41 KB
942 bytes
+++ b/core/misc/drupal.js
@@ -326,7 +326,7 @@ if (window.jQuery) {
   Drupal.url = function (path) {
-    return drupalSettings.path.basePath + drupalSettings.path.scriptPath + drupalSettings.path.pathPrefix + path;
+    return drupalSettings.path.baseUrl + drupalSettings.path.pathPrefix + path;
   };
 

If this change is what is wanted, then this patch is needed to (1) output drupalSettings.path.baseUrl instead of drupalSettings.path.basePath and (2) update the test to look for baseUrl instead of basePath.

If this change is not wanted, then #58 should be used as the starting point, and the hunk above should be reverted.

Status: Needs review » Needs work

The last submitted patch, 62: replace_all_instances-2362227-62.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
49.16 KB
324 bytes

The fail is from a hunk that fell out of the patch for some reason. It is in patches #50 and earlier, but not in #58 and after. I put it back in.

dawehner’s picture

FileSize
2.44 KB
51.4 KB

@tim.plunkett made it clear, that all calls to current_path() as well as _current_path()are removed,
so we can remove that functions itself.

Status: Needs review » Needs work

The last submitted patch, 65: 2362227-65.patch, failed testing.

Status: Needs work » Needs review

tim.plunkett queued 65: 2362227-65.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 65: 2362227-65.patch, failed testing.

mpdonadio’s picture

The patch in #1833932-148: Convert theme_system_compact_link() into a #type link is what is causing this to fail now (it just went in a few hours ago); apply that patch in reverse and #64 and #65 pass. It isn't obvious to me why drupal_get_destination() throws an exception in this context b/c of a route match problem.

The last submitted patch, 64: replace_all_instances-2362227-64.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.24 KB
51.95 KB

Borrowing this hack from _drupal_add_js()

dawehner’s picture

So @tim.plunkett is this patch RTBC?

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

I think @znerol is only right academically, and that the original changes to NullRouteMatch are appropriate for the "real world" we live in. Otherwise we're stuck with hacks like the last interdiff.

That said, this is a step in the right direction, let's do it.

znerol’s picture

Previous patches changed the return value of NullRouteMatch::getRoute() from NULL to a route instance and that would have led to problems with the static cache in CurrentRouteMatche as shown in #44. The patch in #72 does not contain changes to any route mach class, that is what I proposed in #48, so I'm obviously happy with that.

  1. +++ b/core/includes/common.inc
    @@ -1469,18 +1469,20 @@ function _drupal_add_js($data = NULL, $options = NULL) {
    -            'basePath' => base_path(),
    +            'baseUrl' => $request->getBaseUrl() . '/',
    

    That might be an API change for JS code. Should probably be reviewed by frontend folks.

  2. +++ b/core/lib/Drupal/Core/Routing/NullGenerator.php
    @@ -38,6 +38,12 @@ protected function getRoute($name) {
    +    elseif ($name == '<current>') {
    ...
    +    elseif ($name == '<none>') {
    

    Please use the identity operator here.

Once again I want to stress that I am happy with the outcome here.

Wim Leers’s picture

That might be an API change for JS code. Should probably be reviewed by frontend folks.

Only an internal API change, 99% of JS would use Drupal.url(), not any of these individual components.

Reviewing the affected front-end parts:

  1. +++ b/core/misc/drupal.js
    @@ -326,7 +326,7 @@ function DrupalBehaviorError(list, event) {
       Drupal.url = function (path) {
    -    return drupalSettings.path.basePath + drupalSettings.path.scriptPath + drupalSettings.path.pathPrefix + path;
    +    return drupalSettings.path.baseUrl + drupalSettings.path.pathPrefix + path;
       };
    

    Yep, looks solid.

  2. +++ b/core/misc/machine-name.js
    @@ -173,7 +173,7 @@ function machineNameHandler(e) {
    -      return $.get(drupalSettings.path.basePath + 'machine_name/transliterate', {
    +      return $.get(Drupal.url('machine_name/transliterate'), {
    

    Better :)

  3. +++ b/core/modules/views/js/base.js
    @@ -62,7 +62,7 @@
       Drupal.Views.getPath = function (href) {
         href = Drupal.Views.pathPortion(href);
    -    href = href.substring(drupalSettings.path.basePath.length, href.length);
    +    href = href.substring(drupalSettings.path.baseUrl, href.length);
    

    I think dawehner — being a Views maintainer — will have tested this to verify it still works as expected.

znerol’s picture

+++ b/core/modules/views/js/base.js
@@ -62,7 +62,7 @@
-    href = href.substring(drupalSettings.path.basePath.length, href.length);
+    href = href.substring(drupalSettings.path.baseUrl, href.length);

The str.substring() method takes two integer indexes as parameters. The change here is wrong provided that href is a JavaScript string.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

Well spotted! I guess dawehner didn't test it then.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.79 KB
51.84 KB

I think dawehner — being a Views maintainer — will have tested this to verify it still works as expected.

HAHA, no. Now it actually works. Seriously, we need a JS test framework.

Wim Leers’s picture

:)

Yes, we very, very much need a JS testing framework. Hopefully we'll get it in 8.1 or 8.2.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Awesome work - very happy to see _current_path() being removed. One small thing before this can be committed, the change notice needs to improved to have the JS changes too - a quick scan to make sure we've got everything someone would need to know about.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

#81: I don't think that's necessary because (from #76):

Only an internal API change, 99% of JS would use Drupal.url(), not any of these individual components.

Nevertheless, people were used to doing things this way in D7 (but got it wrong ~90% of the time…).

I do agree that this is a public API change though:

+++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
@@ -136,7 +137,7 @@ public static function processAjaxForm(&$element, FormStateInterface $form_state
-   * If #ajax['path'] is set on an element, this additional JavaScript is added
+   * If #ajax['url'] is set on an element, this additional JavaScript is added

Updated the CR for both.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue is a critical task and is allowed per https://www.drupal.org/core/beta-changes. Committed 58480ee and pushed to 8.0.x. Thanks!

  • alexpott committed 58480ee on 8.0.x
    Issue #2362227 by dawehner, mpdonadio, znerol, tim.plunkett, skipyT:...
anavarre’s picture

Should this CR be updated too https://www.drupal.org/node/1659562 ?

Wim Leers’s picture

#85: good point. That CR also still uses url(), which no longer exists…

jbrown’s picture

Status: Fixed » Needs work

The change record doesn't explain what to replace current_path() with. It only explains how to get the current URL.

It needs to explain how to update this:

if ('admin/config/system/composer-manager' == current_path()) {
jbrown’s picture

You can do this:

  if ('admin/config/system/composer-manager' == Url::fromRoute('<current>')->getInternalPath()) {

but the getInternalPath() method is deprecated. What is the proper way to do this?

Berdir’s picture

Use the route, not the path, like this:

\Drupal::routeMatch()->getRouteName() == 'composer_manager.settings'

(Or whatever the route name is ;))

Berdir’s picture

Status: Needs work » Fixed
jbrown’s picture

$element['#ajax']['callback'] no longer works - is $element['#ajax']['url'] now the only way? You can't just specify the function name?

jbrown’s picture

Status: Fixed » Needs work
FileSize
48.21 KB

Yeah - this commit breaks the $element['#ajax']['callback'] functionality. Attached is a screenshot of the problem from the book module. If I revert this commit the problem goes away.

olli’s picture

FileSize
1.58 KB

Here's a fix to the callback option.

jbrown’s picture

Issue tags: +Needs tests
FileSize
3.38 KB

I had to make a few changes to #93 to get it to work.

xjm’s picture

Status: Needs work » Fixed

Please open a followup issue.

xjm’s picture

Issue tags: -Needs tests
jbrown’s picture

xjm’s picture

Status: Fixed » Closed (fixed)

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

joachim’s picture

Status: Closed (fixed) » Active

The change record is not quite complete for this issue.

From https://api.drupal.org/api/drupal/includes!path.inc/function/current_pat...

> http://example.com/drupalfolder/node/306 returns "node/306" while base_path() returns "/drupalfolder/".

The CR suggests this as a replacement:

$url = Url::fromRoute('<current>');
$url->toString();

However, that will get you 'drupalfolder/node/306', not 'node/306'.

Looking at the patch, this is the way to do it:

$current_path = \Drupal::routeMatch()->getRouteName() ? Url::fromRouteMatch(\Drupal::routeMatch())->getInternalPath() : '';

Is that really the most succinct way?

dawehner’s picture

@joachim
The point is that you would almost never have to use the path, but you should rather just pass around the URL everywhere.

joachim’s picture

Sure, but in updating modules to D8, the first step will often be to just replace deprecated functions, and change the way things are handled as a second step. So for example in admin_menu, I had:

  // Performance: Skip this entirely for AJAX requests.
  $current_path = current_path();
  if (strpos($current_path, 'js/') === 0) {
    return;
  }

I can't use URL::__toString() because that gives a subfolder if there is one.

Shall I update the CR with $current_path = \Drupal::routeMatch()->getRouteName() ? Url::fromRouteMatch(\Drupal::routeMatch())->getInternalPath() : ''; as the 'replacement' for current_path()?

Berdir’s picture

Status: Active » Fixed

Sometimes it's just not possible to do a direct 1:1 conversion.

Drupal 8 actually gives you much better tools for that kind of check, which is actually very limited in 7.x (99% of the ajax requests go either through system/ajax or views/ajax I'd say and that won't match).

What you want in 8.x is the request format. $request->getRequestFormat(). See how MainContentViewSubscriber uses it, 4 common formats are html, drupal_ajax, drupal_dialog and drupal_modal, and you probably just want to do something for html?

I don't think that use case really belongs in a change record, as it is very special...

jrockowitz’s picture

The most succinct (and reasonable) replacement for current_path() that I have found is Url::fromRoute('<current>')->getInternalPath().

Url::getInternalPath() and Entity::getSystemPath() are deprecated but also needed by the path.module's PathWidget to save a URL alias' source as its internal path.

The path.module might be reworked while Drupal 8 is in beta (see #2336597) but it would still be using Entity::getSystemPath() with the PathWidget.

I am not sure how 'internal paths' could be completely deprecated in D8 core.

Status: Fixed » Closed (fixed)

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

alexpott’s picture

Created #2426457: Remove request_path as a followup.