Flag links redirect to the system path rather than the actual path the user was on

+++ b/src/Controller/ActionLinkController.php
@@ -80,6 +81,7 @@ class ActionLinkController extends ControllerBase implements ContainerInjectionI
+      drupal_set_message($flag->getFlagMessage(), 'status');

@@ -110,6 +112,7 @@ class ActionLinkController extends ControllerBase implements ContainerInjectionI
+      drupal_set_message($flag->getUnflagMessage(), 'status');

Why these two additions? They don't look related to the issue...

I don't understand the issue here. This should work fine, it also doesn't matter if you use pathauto or not. Pathauto generates aliases, it has 0 impact on url generation at runtime.

Sending redirects and existing is wrong and against the API.

Can you also list your steps to reproduce?

I'm curious why you need to build the flag link yourself in the first place.

Have you used the pseudofield setting? What configurability do you need that that doesn't give you?

I also have an issue with the flag not respecting the URL alias (PathAuto).

When the flag is completed/uncompleted then it redirects to the internal path instead of the alias..
Destination argument is not using the url alias created with pathauto, this causes some unwanted behaviour as sample breadcrumbs (base on url) - beside the end users noticing that the url changed.

Using the Ajax option would resolve the issue (because it won't be redirection), but unfortunately for some restrictions we can not use it.

How to replicate it:
Create a content type, include a flag, create content with an alias, when you try to click the flag link you can see something like this:
/flag/flag/task_completion/136?destination=/node/142&token=UglbmpbkSbO8cspQ4f_4jJAoX2htrizCNULAVlygqqk
instead of
/flag/flag/task_completion/136?destination=/dashboard&token=UglbmpbkSbO8cspQ4f_4jJAoX2htrizCNULAVlygqqk

Same behaviour if you add try to use the flag link in views.

Attached a patch to use alias instead of internal path, if an alias exist it would use it, if not it would maintain the regular path.

As you can see on the test fails, getPath and getInternalPath() work differently. getPath() returns the path prefix if drupal runs in a subfolder.

If anything, you need to use '/' . getInternalPath() for the alias.

But this is a bit strange, I'd expect the destination handling will then forward to the alias, if not then that seems like an issue with that, not with this module and will affect many other things too?

As a workaround, you could use the canonical redirect settings from the redirect module

I did a simpletest.me only core and the flag module.

To make sure that there was not other module causing the issue. Not even enabled PathAuto, used the default path option to update the alias.

And following the same steps added in #10, the issue of the destination argument not using alias is replicable.

Ok so clearly when we're getting the current URL to put into the link's query destination string, we're getting the system path, not the actual current path.

Here's our code:

  protected function getDestination() {
    $current_url = Url::fromRoute('<current>');
    $route_params = $current_url->getRouteParameters();

    if (isset($route_params['destination'])) {
      return $route_params['destination'];
    }

    return $current_url->getInternalPath();
  }

getInternalPath() is clearly wrong.

It occurs to me that getAlias() -- if such a thing exists -- might not be right either, as there might be more than one alias for the current path.

So, what is the RIGHT way to get a destination query string for the current path? Surely that's something core provides?

Hi @joachim,

And there is something that I believe it is a major issue... the language prefix is never set either.

So if you are in a FR section the flag destination path never gets the language prefix. Causing a redirection to the default language page.
/fr/flag/flag/task_completion/136?destination=/node/142&token=Uglbmpbk
instead of:
/fr/flag/flag/task_completion/136?destination=fr/node/142&token=Uglbmpbk
or even better:
/fr/flag/flag/task_completion/136?destination=fr/path_alias&token=Uglbmpbk

We needed a way to have the path with the correct language and alias, so for now we changed the line:

return $current_url->getInternalPath();

for this:

//Return Alias and Language prefix instead of internal path
$language = \Drupal::languageManager()->getCurrentLanguage()->getId();
$current_path = \Drupal::service('path.current')->getPath();
$alias = \Drupal::service('path.alias_manager')->getAliasByPath($current_path, $language );
//TO-DO -- Better way to get the language prefix.
if($language != 'en'){
  $alias = "/". $language . $alias;
}
return $alias;

I know there is not an elegant way to get the language prefix, but at least is working and the alias is language base too.

That's really overthinking it! It's not flag's job to know how paths are put together, and indeed, about the different ways in which language URLs can be constructed.

All we care about is that the user is currently on www.domain.com/foo/bar and that they should go back there once they've performed the flagging action.

It looks like \Drupal::request()->getRequestUri(); will get us what we want in getDestination() -- obviously instead of ::request() we need to inject the service.

If anything, then the problem isn't in the code that generates the destination string but where we actually do the redirect. That is supposed to run it through the usual outbound processing which should take care of all that. I'll try to have a look at what's exactly happening.

> I also noticed no "flashdata" (or w/e drupal calls them) are displayed so I added 2 flashdata and attached them to the un/flag methods.

Split this off to #2834455: [Regression] Flag/unflag messages don't show for reload and confirm link types.

That's an interesting point. I'm expecting \Drupal\Core\Routing\RedirectDestination::get to totally return a path alias, given that it calls out to ::generateFromRoute().

@joachim

You are completely right, I was overthinking,
return \Drupal::request()->getRequestUri();
Instead of
return $current_url->getInternalPath();
Using the uri resolves my issues, it works even when I'm using the flag in the front page (destination '/').

Could you explain the service injection?

obviously instead of ::request() we need to inject the service.

Thank you so much!
Blanca

> obviously instead of ::request() we need to inject the service.

The \Drupal:: static is for procedural code. In classes such as this one, the service should be injected instead: the class constructor and factory method should together set the service on the object. Then instead of \Drupal:foo(), we have $this->someService->foo().

Have a look at what the \Drupal::request() method does, that will probably be a wrapper around a call to a service.

> That is supposed to run it through the usual outbound processing which should take care of all that.

There's a potential problem there if the current path has more than 1 alias. Suppose we're on /alias-b. The processing could give us /alias-a for /system-path instead.

@Blanca.Esqueda

\Drupal::request()->getRequestUri()

is a function call to this

  public static function request() {
    return static::getContainer()->get('request_stack')->getCurrentRequest();
  }

From the container it gets services, in this case 'request_stack'

using \Drupal::Xyz() is an anti pattern. Injecting the service directly is done by adding the service to the construct function

looking at the ActionLinkTypeBase definition below the create() method pulls in the 'current_user' service and adds it to a list of other things that will when the object is created become the parameters passed into the __construct() method.

if you add request_stack service to the end of the list in the create() method and then add an extra parameter to the constructor then I hope you will see the pattern

If you have any questions just let me know...

  /**
   * Build a new link type instance and sets the configuration.
   *
   * @param array $configuration
   *   The configuration array with which to initialize this plugin.
   * @param string $plugin_id
   *   The ID with which to initialize this plugin.
   * @param array $plugin_definition
   *   The plugin definition array.
   * @param \Drupal\Core\Session\AccountInterface $current_user
   *   The current user.
   */
  public function __construct(array $configuration, $plugin_id, array $plugin_definition, AccountInterface $current_user) {
    parent::__construct($configuration, $plugin_id, $plugin_definition);
    $this->configuration += $this->defaultConfiguration();
    $this->currentUser = $current_user;
  }

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
    return new static(
      $configuration,
      $plugin_id,
      $plugin_definition,
      $container->get('current_user')
    );
  }

@Blanca.Esqueda, @joachim

I've been having a similar problem with the flag respecting path aliases on destination redirect and found this approach to work.

By Replacing the original call to getInternalPath

return $current_url->getInternalPath();

with a call to toString the current url

return $current_url->toString();

This will allow the destination to always respect the current url path that is being accessed prior to triggering the flag whether or not it is an alias path.

Thanks for the patch!
(Though remember to set an issue to 'needs review' so people know to look at your patch.)

This fixes the problem, but it's not as good a fix as using getRequestUri(), as that will also preserve any query string in the current URL.

(The use case for that is a view that has exposed filters and is showing flag links. You might apply some filters, then flag something. You'd expect to return to the filtered view you were on.)

Not sure how I failed to spot this hadn't been classed as a bug...

Hi,

I don't understand why not using the 'redirect.destination' service instead?

It already does what you need no? Implementing the request stack, preserving the query string, take care of alias and langcode etc.. etc..

Or am I missing a point?

Just in case I added a patch that implements the 'redirect.destination' service. Since it is a different approach I did not provide an interdiff.
(If necessary I'll take a look at the tests (I expect it to fail))

I don't understand why not using the 'redirect.destination' service instead?

Does that handle the destination query parameter as well?

Also it seems like it changes the redirect, breaking the test:

doEditFlagField
fail: [Other] Line 156 of modules/contrib/flag/src/Tests/LinkTypeFieldEntryTest.php:
Expected 'http://localhost/checkout/flag/details/edit/dcbfutaw/1?destination=node/1' matches current URL (http://localhost/checkout/flag/details/edit/dcbfutaw/1?destination=/chec...).
Value 'http://localhost/checkout/flag/details/edit/dcbfutaw/1?destination=/chec...' is equal to value 'http://localhost/checkout/flag/details/edit/dcbfutaw/1?destination=node/1'.

Does that handle the destination query parameter as well?

Sorry I'm not sure to understand the question but I'll try to answer.

It is a service that provide helpers for redirect destinations. If you take a look at the get method you can see that it handle the destination query parameter :

  /**
   * {@inheritdoc}
   */
  public function get() {
    if (!isset($this->destination)) {
      $query = $this->requestStack->getCurrentRequest()->query;
      if (UrlHelper::isExternal($query->get('destination'))) {
        $this->destination = '/';
      }
      elseif ($query->has('destination')) {
        $this->destination = $query->get('destination');
      }
      else {
        $this->destination = $this->urlGenerator->generateFromRoute('<current>', [], ['query' => UrlHelper::filterQueryParameters($query->all())]);
      }
    }

    return $this->destination;
  }

I'll take a look a the tests as soon as I could.

I upload a patch to try passing the test:

doEditFlagField
fail: [Other] Line 156 of modules/contrib/flag/src/Tests/LinkTypeFieldEntryTest.php:
Expected 'http://localhost/checkout/flag/details/edit/jedbioim/1?destination=node/1' matches current URL (http://localhost/checkout/flag/details/edit/jedbioim/1?destination=/chec...).
Value 'http://localhost/checkout/flag/details/edit/jedbioim/1?destination=/chec...' is equal to value 'http://localhost/checkout/flag/details/edit/jedbioim/1?destination=node/1'.

I don't have the same behavior on my test environment because there is no checkout on the URL.

It seems that the other fails are not related to this patch since they exist without it on my environment but I will take a look to be sure.

Sorry I forgot to change the status..

This patch take into account the /checkout/ on drupalci for the LinkTypeFieldEntrydoEditFlagFieldield() test. To have the correct destination to assert it needs to construct it using

Url::fromRoute('entity.node.canonical', ['node' => $this->nodeId]);

The other fails are not related to this patch since they exist without it.

The other fails are not related to this patch since they exist without it.

Yep those issue are known failures and set to be fixed in the issue above.

Yep those issue are known failures and set to be fixed in the issue above.

Thanks! I was looking for this issue!

Sorry I included the wrong issue as our branch blocker. Let us call it a dozy sunday morning thing :)

Fixed! Thanks all!