URL: Fix getQueryString incorrect handling of hash fragment

packages/url/CHANGELOG.md

@@ -1,5 +1,9 @@
 ## Master
 
+### Bug Fixes
+
+- `getQueryString` now correctly considers hash fragments when considering whether to return a query string. Previously, `getQueryString( 'https://example.com/#?foo' )` would wrongly return `'foo'` as its result. A hash fragment is always the last segment of a URL, and the querystring must always precede it ([see reference specification](https://url.spec.whatwg.org/#absolute-url-with-fragment-string)).
+
 ## 2.11.0 (2020-02-10)
 
 ### Bug Fixes

packages/url/README.md

@@ -178,8 +178,7 @@ Returns the query string part of the URL.
 _Usage_
 
 ```js
-const queryString1 = getQueryString( 'http://localhost:8080/this/is/a/test?query=true#fragment' ); // 'query=true'
-const queryString2 = getQueryString( 'https://wordpress.org#fragment?query=false&search=hello' ); // 'query=false&search=hello'
+const queryString = getQueryString( 'http://localhost:8080/this/is/a/test?query=true#fragment' ); // 'query=true'
 ```
 
 _Parameters_

packages/url/src/get-query-string.js

@@ -5,15 +5,18 @@
  *
  * @example
  * ```js
- * const queryString1 = getQueryString( 'http://localhost:8080/this/is/a/test?query=true#fragment' ); // 'query=true'
- * const queryString2 = getQueryString( 'https://wordpress.org#fragment?query=false&search=hello' ); // 'query=false&search=hello'
+ * const queryString = getQueryString( 'http://localhost:8080/this/is/a/test?query=true#fragment' ); // 'query=true'
  * ```
  *
  * @return {string|void} The query string part of the URL.
  */
 export function getQueryString( url ) {
-	const matches = /^\S+?\?([^\s#]+)/.exec( url );
-	if ( matches ) {
-		return matches[ 1 ];
+	let query;
+	try {
+		query = new URL( url ).search.substring( 1 );
+	} catch ( error ) {}
+
+	if ( query ) {
+		return query;
 	}
 }

packages/url/src/test/index.test.js

@@ -272,11 +272,6 @@ describe( 'isValidPath', () => {
 
 describe( 'getQueryString', () => {
 	it( 'returns the query string of a URL', () => {
-		expect(
-			getQueryString(
-				'https://user:password@www.test-this.com:1020/test-path/file.extension#anchor?query=params&more'
-			)
-		).toBe( 'query=params&more' );
 		expect(
 			getQueryString(
 				'http://user:password@www.test-this.com:1020/test-path/file.extension?query=params&more#anchor'
@@ -305,16 +300,21 @@ describe( 'getQueryString', () => {
 				'https://andalouses.example/beach?foo[]=bar&foo[]=baz'
 			)
 		).toBe( 'foo[]=bar&foo[]=baz' );
-		expect( getQueryString( 'test.com?foo[]=bar&foo[]=baz' ) ).toBe(
+		expect( getQueryString( 'https://test.com?foo[]=bar&foo[]=baz' ) ).toBe(
 			'foo[]=bar&foo[]=baz'
 		);
-		expect( getQueryString( 'test.com?foo=bar&foo=baz?test' ) ).toBe(
-			'foo=bar&foo=baz?test'
-		);
+		expect(
+			getQueryString( 'https://test.com?foo=bar&foo=baz?test' )
+		).toBe( 'foo=bar&foo=baz?test' );
 	} );
 
 	it( 'returns undefined when the provided does not contain a url query string', () => {
 		expect( getQueryString( '' ) ).toBeUndefined();
+		expect(
+			getQueryString(
+				'https://user:password@www.test-this.com:1020/test-path/file.extension#anchor?query=params&more'
+			)
+		).toBeUndefined();
 		expect(
 			getQueryString( 'https://wordpress.org/test-path#anchor' )
 		).toBeUndefined();
@@ -326,11 +326,10 @@ describe( 'getQueryString', () => {
 		).toBeUndefined();
 		expect( getQueryString( 'https://wordpress.org/' ) ).toBeUndefined();
 		expect( getQueryString( 'https://localhost:8080' ) ).toBeUndefined();
-		expect( getQueryString( 'https://' ) ).toBeUndefined();
-		expect( getQueryString( 'https:///test' ) ).toBeUndefined();
-		expect( getQueryString( 'https://#' ) ).toBeUndefined();
-		expect( getQueryString( 'https://?' ) ).toBeUndefined();
-		expect( getQueryString( 'test.com' ) ).toBeUndefined();
+		expect( getQueryString( 'invalid' ) ).toBeUndefined();
+		expect(
+			getQueryString( 'https://example.com/empty?' )
+		).toBeUndefined();
 	} );
 } );