UrlGenerator:processPath() doesn't use altered query parameters

Could we also do something like this?

1. +++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
   @@ -299,19 +301,18 @@ public function generateFromRoute($name, $parameters = array(), $options = array
   -      $query = $query_params ? '?' . http_build_query($query_params, '', '&') : '';
   +      $query = !empty($options['query']) && is_array($options['query']) ? '?' . UrlHelper::buildQuery($options['query']) : '';
   

   Why do we need to check for the array bit? It seems to be a clear developer error when query is not an array. What about adding an assertion for that?

2. +++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
   @@ -561,4 +562,52 @@ public function getRouteDebugMessage($name, array $parameters = array()) {
   +    // @todo Fix all the instances in core where query is not set to an array.
   +    if (!is_array($query)) {
   

   OH I see, meh

Fixed the failures.

Meh

This issue is a blocker for #1810394: Site configuration with domain based language negotiation results in redirecting authenticated users to a different domain when accessing a content entity route for translation language different from the interface language where we define a new outbound processor and have to manually alter the path instead of only adding a new query parameter in the options array.

re-rolling patch.

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -561,4 +605,52 @@ public function getRouteDebugMessage($name, array $parameters = array()) {
+
+  /**
+   * Merge query string from the path and options array.
+   *
+   * @param string $path
+   *   The path. If it contains a query string, these parameters should already
+   *   be encoded.
+   * @param array $query
+   *   An array of key/value pairs. These will be passed to
+   *   \Drupal\Component\Utility\UrlHelper::buildQuery().
+   *
+   * @return string
+   *   The complete path including the query string.
+   */
+  protected function mergeQueryString($path, $query) {
+    // @todo Fix all the instances in core where query is not set to an array.
+    if (!is_array($query)) {
+      $string = $query;
+      $query = [];
+      parse_str($string, $query);
+    }
+
+    $path_query = [];
+    if (($query_pos = strpos($path, '?')) && $query_pos !== FALSE) {
+      // Parse the query that is part of the string into an array.
+      $actual_path = substr($path, 0, $query_pos);
+      $query_string = ltrim(substr($path, $query_pos), '?');
+      parse_str($query_string, $path_query);
+      $query_string = '?' . $query_string;
+    }
+    else {
+      $actual_path = $path;
+      $query_string = '';
+    }
+
+    // Make sure not to include the same parameter more than once.
+    $query = array_diff_key($query, $path_query);
+
+    // Build the path.
+    $full_path = $actual_path . $query_string;
+    if (!empty($query)) {
+      $separator = $query_string ? '&' : '?';
+      $full_path .= $separator . UrlHelper::buildQuery($query);
+    }
+
+    return $full_path;
+  }

Note: We can remove this entirely

@dawehner:
Done.

Cool, thank you. Asked @pwolanin for a review.

This looks wrong:

+      parse_str($options['query'], $options['query']);

The query should always be an array until the final rendering.

This method name doesn't make much sense to me - what's it supposed to be doing?

generatePathWithString()

It looks like fixing this bug properly requires more rewriting of the internals of the generator. In particular, this method call should change and not pass in the query params and should get back out any extra parameters somehow to add to them.

$path = $this->getInternalPathFromRoute($name, $route, $parameters, $query_params);

The query should always be an array until the final rendering.

Drupal sucks, see issue summary.

So, we should try to fix this properly. In the worst case, we need a BC shim to check the result of outbound processing for a "?" to see if some crufty code appending a string query that needs to be merged with the array data.

Rerolling, because the patch couldn't be applied anymore as changes in UrlGenerator have been commited.

Why this patch here is failing is better explained in #2618510: Add test ensuring LanguageNegotiationContentEntity is correctly processing the query options.

The issue that was blocked on this is closed.

I added a small patch that partially fixes this issue, as this does not seem fixed in 8.1.2.

Disregard..

Setting back to "needs review" as only the patch from #51 is to be considered.

@hchonov Does #51 still apply?

@dawehner: no, will work on the reroll.

Subscribing as OutboundPathProcessorInterface currently doesn't allow to change the url query.
Needs a reroll.

Tried to reroll and simplify the patch a bit

One bugfix

This issue will be fixed as part of #2809789: UrlGenerator fails to urlencode aliases, incorrectly encodes "system" path

I'm working with 8.2.x and confirm that patch in #2809789-40: UrlGenerator fails to urlencode aliases, incorrectly encodes "system" path fix this issue, and it was committed to 8.3.x in #2809789-46: UrlGenerator fails to urlencode aliases, incorrectly encodes "system" path so maybe we should mark this as it wont fix.

ok, so we can mark this as fixed since it was resolved in the other issue

Thanks @pwolanin! I think duplicate makes the most sense here.