Contrib.social

Url generation doesn't support non-default paths

Open on Drupal.org →
Created on 22 March 2023, about 2 years ago
Updated 23 August 2023, over 1 year ago

Problem/Motivation

Some use cases would prefer instead of `/api` the api documentation be at `/whatever/documentation` and all the dependent pages. Also, the Drupal install could be in a subfolder - in which case URL structure would be `/subfolder/api`.

That is easy to do with a RouteSubscriber for the base routes in a custom module - and I don't think it makes sense to have that degree of configuration exposed in a form etc although I can see that arguable both ways. However, currently almost all links will be wrong for a custom path, and some links will be wrong for a subfolder install.

Project links, easy to change the url pattern so not concerned about.

The problem with most of the rest of the links is they use Url::fromUri('internal:[path]') instead of Url::fromRoute('route', [parameters]) - or in some twig cases just /api (which will fail in subfolder installs).

Steps to reproduce

Either install drupal in a subfolder or use an Event Subscriber ex: (very naive example)

  protected function alterRoutes(RouteCollection $collection) {
    foreach ($collection->all() as $route) {
      if ($route->getRequirement('_permission') === 'access API reference') {
        $route->setPath(str_replace('/api/', '/work/drupal-api/', $route->getPath()));
      }
    }
  }

  /**
   * {@inheritdoc}
   */
  public static function getSubscribedEvents(): array {
    $events = parent::getSubscribedEvents();
    $events[RoutingEvents::ALTER] = ['onAlterRoutes', -300];
    return $events;
  }

Proposed resolution

Use Drupal routes in link generation

Remaining tasks

Discuss if this is the right direction. At which point,
Create tests that prove it works/ensures it stays working.
Update existing tests to be more generic.

User interface changes

None.

API changes

Breaking changes for sure. A couple functions in Formatter are changed to return Url objects instead of strings.

Data model changes

None

🐛 Bug report
Status

Fixed

Version

2.0

Component

Code

Created by

🇨🇦Canada nickdickinsonwilde Victoria, BC (T'So-uke lands)

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @nickdickinsonwilde
  • @nickdickinsonwilde opened merge request.
  • Status changed to Needs review about 2 years ago
  • Comment about 2 years ago →
    🇨🇦Canada nickdickinsonwilde Victoria, BC (T'So-uke lands)
  • Comment about 2 years ago →
    🇪🇸Spain fjgarlin

    The "/api" is heavily ingrained in the module (both D7 and D10). It's not just the link generation, but also the whole routing system (see api.routing.yml) for the module.

    Your EventSubscriber solution is a good way to get around it, but as you say, that's only part of it. For the generated links, you could also create an OutboundPathProcessor to alter the links to have the desired path prefix. I think that will get you 100% there with just a few files.

    I'm not sure if it's worth putting all the effort on refactoring routes, code, tests on the module when there is a workaround, but I'm happy to hear more thoughts.

  • Comment about 2 years ago →
    🇪🇸Spain fjgarlin

    From the example provided here https://www.drupal.org/node/2238759 → , and your code, here is an (untested) suggestion:

    function processOutbound($path, &$options = array(), Request $request = NULL) {
   if (strpos($path, '/api/') === 0) {
      return str_replace('/api/', '/work/drupal-api/', $path);
   }

   return $path;
}
  • Status changed to Closed: works as designed almost 2 years ago
  • Comment almost 2 years ago →
    🇨🇦Canada nickdickinsonwilde Victoria, BC (T'So-uke lands)

    Thanks for the feedback. I personally feel that it is very much better to not presume paths and use proper Urls instead of strings for paths.

    That said, yes, I was able to switch to using both an inbound and outbound path processor and made it work for my personal use case without patching the module. So calling it works as designed.

  • Status changed to Fixed over 1 year ago
  • Comment over 1 year ago →
    🇪🇸Spain fjgarlin

    While working on some other tasks related to test failures, I needed to change a few things regarding path generation, which made me remember this issue.

    The MR was missing a few places where ::objectUrl was being used, but the principle and the proof of concepts were still valid, so I am actually including a bunch of the changes suggested here in the work that I am doing.

    I don't know if I'll be able to include all of them, but I am definitely trying to. I need to test things all over the place (and good thing we also have functional tests!!).

    The MR with the work and commit history that I am doing is here: https://git.drupalcode.org/project/api/-/merge_requests/12/diffs

    I am going to mark this issue as Fixed and give credit as the changes will be in the module very soon.
    Thanks!

  • Comment over 1 year ago →
    System Message

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

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024