Move url related functions to a new Url component

+++ b/core/lib/Drupal/Component/Utility/HttpQuery.php
@@ -0,0 +1,68 @@
+   * This differs from http_build_query() as we need to rawurlencode() (instead of
+   * urlencode()) all query parameters.

Why? No one has ever explained to me why we have to do something different here.

+++ b/core/lib/Drupal/Component/Utility/HttpQuery.php
@@ -0,0 +1,68 @@
+  public static function getArray($query) {
+    $result = array();
+    parse_str($query, $result);
+    return $result;

This is too small to bother making a utility function. Really.

+++ b/core/lib/Drupal/Component/Utility/HttpQuery.php
@@ -0,0 +1,68 @@
+   * This differs from http_build_query() as we need to rawurlencode() (instead of
+   * urlencode()) all query parameters.

Why? No one has ever explained to me why we have to do something different here.

rawurlencode() is RFC3986 compliant, and as a consequence RFC3987 compliant. The latter defines the required format of "URLs" in HTML5.

urlencode() is almost the same as rawurlencode(), except that it encodes spaces as "+" instead of "%20". This makes its result non compliant to RFC3986 and as a consequence non compliant to RFC3987 and as a consequence not valid as a "URL" in HTML5.

That said, http_build_query() has an optional $enc_type parameter since PHP 5.4, that allows it to generate URLs compliant with HTML5.

Sigh. If we need it essentially as a BC wrapper, then we should document that (per #4) and if on PHP 5.4 just use http_build_query() directly. That way when we eventually move to just 5.4 we can drop this function entirely.

The generator already has an httpBuildQuery() method:
http://drupalcode.org/project/drupal.git/blob/2432c02138861423b7db365265...

Imo drupal_parse_url and httpBuildQuery should be together in one component

that too..anything that has to do with query/url besides generating should be in the same component...so that people can find them immediately..maybe merge UrlValidator too

Ok, how about we move the whole lot of methods into one Url class? This patch is still rough around the edges, be warned.

retitling

+++ b/core/lib/Drupal/Component/Utility/Url.php
@@ -20,6 +20,122 @@ class UrlValidator {
+    // Set defaults, if none given.
+    if (!isset($query)) {
+      $query = $_GET;
+    }

No way. $query should be required. Code in Component should not be touching global anything.

I said be warned, and rough around the edges. I thought it was pretty obvious I just quickly c & p'ed a few things.

ok, I've moved some more things. I'm not sure about moving _external_url_is_local, as it needs the base path. Thoughts?

I also spoke to Paris about merging in #2020811: Move _external_url_is_local and url_is_external to UrlValidator and putting everything in this Url class.

Seems the version_compare logic was backwards.

Sorry, elementary error there.

ok, I've moved some more things. I'm not sure about moving _external_url_is_local, as it needs the base path. Thoughts

you can see my patch over @ #2020811: Move _external_url_is_local and url_is_external to UrlValidator
Just add one more parameter. $base_url

Thanks for the review! Yep, accidentally left that line commented out :) Added the @see, @deprecated tags, and fixed up the other part of the doc block (I think). I also moved everything from UrlValidatorTest to UrlTest.

I have left check_url() for now, as it uses check plains the url string. Do you think we should move that into this class too? and just rely on string? or keep it out of there?

Sorry Paris, I x posted with your comment, here is a new patch with that rolled in.

Great, looks awesome
just a thing which will make bot fail probably

+++ b/core/includes/common.incundefined
@@ -624,9 +563,11 @@ function drupal_parse_url($url) {
-  return str_replace('%2F', '/', rawurlencode($path));
+  Url::encodePath($path);

you dont return the value anymore

Thank you!

That'll do it!

Thank you, I changed drupal_get_destination to use the Request object instead. I also changed all the data provider docblocks, so they are consistent.

one $_GET escaped:P die!
heh
damiankloip++

@todo is not an accpetable issue summary :D

Updated issue summary...not sure what the status should be...letss put it in needs review

Proper issue summary

Back to RTBC. I couldn't find anything in the new class that violated Component rules, so...

+++ b/core/lib/Drupal/Component/Utility/Url.phpundefined
@@ -0,0 +1,373 @@
+  /**
+   * Splits a URL-encoded query string into an array.
+   *
+   * @param string $query
+   *   The query string to split.
+   *
+   * @return array
+   *   An array of URL decoded couples $param_name => $value.
+   */
+  public static function getArray($query) {
+    $result = array();
+    parse_str($query, $result);
+    return $result;
+  }

+++ b/core/includes/common.incundefined
@@ -479,16 +457,11 @@ function drupal_get_query_parameters(array $query = NULL, array $exclude = array
 function drupal_get_query_array($query) {
-  $result = array();
-  if (!empty($query)) {
-    foreach (explode('&', $query) as $param) {
-      $param = explode('=', $param);
-      $result[$param[0]] = isset($param[1]) ? rawurldecode($param[1]) : '';
-    }
-  }
-  return $result;
+  return Url::getArray($query);
 }

As Url::getArray just calls the native PHP function we just use this instead so lets deprecate this for parse_str and not Just Another Wrapper (tm)

yeah i can reproduce the failures in PHP 5.4, so yes lets just stick with the old code for now.
removing the wrapper is awesome though!
thanks:)

Needs a reroll

curl https://drupal.org/files/vdc-2010024-45.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 44121  100 44121    0     0  33760      0  0:00:01  0:00:01 --:--:-- 38976
error: patch failed: core/includes/common.inc:3
error: core/includes/common.inc: patch does not apply

Pure git reroll.

Committed 91c32e3 and pushed to 8.x. Thanks!

We need a change notice to announce the removal of drupal_get_query_array and to tell people to use the built in parse_str()

yay!

Moved to routing system per #2050763-16: Refine "base system" component (notes on refactoring of "base system" category here: https://docs.google.com/a/acquia.com/spreadsheet/ccc?key=0AusehVccVSq2dF...).

I shifted the list of functions to the body, not the title, so that it's actually readable. :-) You should still be able to search by the function name, though. We're good.

We need a change notice to announce the removal of drupal_get_query_array and to tell people to use the built in parse_str()

This is still missing from the change notice (https://drupal.org/node/2079005).

(Coming here from #2143461: drupal_get_query_array encodes not enough if query parameter contains a equal sign (=); was trying to decide if that patch needed to go into Drupal 8 too and had to read this issue rather than the change notice to figure out that it didn't...)

Tagging for the task of updating the change record as described in #55.

Change record updated.

Removing tags

Thanks @SebCorbin! I think we can keep this tag for posterity.