From 4dd2b8d885d847ded34eabc2c05112d35c4ba929 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Fri, 13 May 2022 18:14:47 +0100 Subject: UrlUtils: Simplify and increase test coverage Follows-up I31f1403ab40c79ab270c417. * Make the test cases less dynamic. It was approaching the same kind of complexity as the source code, thus reducing confidence in the test. * Make the test cases more explicit. A good test can be read without executing it and show plainly what the expected output are. Use little to no indirection, favouring small amounts of duplication when in doubt. * Increase coverage of the "non-standard HTTPS port" configuration, by removing the two hardcoded assertions for it, in favour of including it in the overal test matrix and applying the same set of default cases to it as the other configurations under test. * There are now 306 instead of 2355 generated test cases, but the code coverage of UrlUtils.php remains identical at 98.28% (171/174 lines). Change-Id: I755c017964757b9aab5ebeabd80be5664be48798 --- includes/utils/UrlUtils.php | 3 +- .../unit/includes/utils/UrlUtilsProviders.php | 201 +++++++++------------ 2 files changed, 89 insertions(+), 115 deletions(-) diff --git a/includes/utils/UrlUtils.php b/includes/utils/UrlUtils.php index 55b89e400ee1..f21b9e734d59 100644 --- a/includes/utils/UrlUtils.php +++ b/includes/utils/UrlUtils.php @@ -144,7 +144,8 @@ class UrlUtils { $defaultProto = $serverProto ?? PROTO_HTTP; } - $defaultProtoWithoutSlashes = $defaultProto === null ? '' : substr( $defaultProto, 0, -2 ); + // @phan-suppress-next-line PhanTypeMismatchArgumentNullableInternal T308355 + $defaultProtoWithoutSlashes = $defaultProto === PROTO_FALLBACK ? '' : substr( $defaultProto, 0, -2 ); if ( substr( $url, 0, 2 ) == '//' ) { $url = $defaultProtoWithoutSlashes . $url; diff --git a/tests/phpunit/unit/includes/utils/UrlUtilsProviders.php b/tests/phpunit/unit/includes/utils/UrlUtilsProviders.php index 0c4cc2df659f..65c13c97c358 100644 --- a/tests/phpunit/unit/includes/utils/UrlUtilsProviders.php +++ b/tests/phpunit/unit/includes/utils/UrlUtilsProviders.php @@ -45,132 +45,105 @@ class UrlUtilsProviders { ]; } - /** - * Helper method to reduce nesting in provideExpand() - * - * @return Generator - */ - private static function provideExpandCases(): Generator { + public static function provideExpand() { + $modes = [ 'http', 'https' ]; $servers = [ 'http://example.com', 'https://example.com', '//example.com', ]; + $defaultProtos = [ + 'http' => PROTO_HTTP, + 'https' => PROTO_HTTPS, + 'protocol-relative' => PROTO_RELATIVE, + 'fallback' => PROTO_FALLBACK, + 'canonical' => PROTO_CANONICAL, + ]; - foreach ( $servers as $server ) { - foreach ( [ 'http://example2.com', 'https://example2.com' ] as $canServer ) { - foreach ( [ 'http://internal.com', 'https://internal.com' ] as $intServer ) { - foreach ( [ 'http', 'https' ] as $fallbackProto ) { - foreach ( [ 111, 443 ] as $httpsPort ) { - $options = [ - UrlUtils::SERVER => $server, - UrlUtils::CANONICAL_SERVER => $canServer, - UrlUtils::INTERNAL_SERVER => $intServer, - UrlUtils::FALLBACK_PROTOCOL => $fallbackProto, - UrlUtils::HTTPS_PORT => $httpsPort, - ]; - foreach ( self::DEFAULT_PROTOS as $protoDesc => $defaultProto ) { - yield [ $options, $protoDesc, $defaultProto ]; - } + foreach ( $modes as $fallbackProto ) { + foreach ( $servers as $server ) { + foreach ( $modes as $canServerMode ) { + $canServer = "$canServerMode://example2.com"; + $options = [ + UrlUtils::SERVER => $server, + UrlUtils::CANONICAL_SERVER => $canServer, + UrlUtils::HTTPS_PORT => 443, + UrlUtils::FALLBACK_PROTOCOL => $fallbackProto, + ]; + foreach ( $defaultProtos as $protoDesc => $defaultProto ) { + $case = "fallback: $fallbackProto, default: $protoDesc, server: $server, canonical: $canServer"; + yield "No-op fully-qualified http URL ($case)" => [ + 'http://example.com', + $options, $defaultProto, + 'http://example.com', + ]; + yield "No-op fully-qualified https URL ($case)" => [ + 'https://example.com', + $options, $defaultProto, + 'https://example.com', + ]; + yield "No-op rootless path-only URL ($case" => [ + "wiki/FooBar", + $options, $defaultProto, + 'wiki/FooBar', + ]; + + // Determine expected protocol + if ( $protoDesc === 'protocol-relative' ) { + $p = ''; + } elseif ( $protoDesc === 'fallback' ) { + $p = "$fallbackProto:"; + } elseif ( $protoDesc === 'canonical' ) { + $p = "$canServerMode:"; + } else { + $p = $protoDesc . ':'; + } + yield "Expand protocol-relative URL ($case)" => [ + '//wikipedia.org', + $options, $defaultProto, + "$p//wikipedia.org", + ]; + + // Determine expected server name + if ( $protoDesc === 'canonical' ) { + $srv = $canServer; + } elseif ( $server === '//example.com' ) { + $srv = $p . $server; + } else { + $srv = $server; } + yield "Expand path that starts with slash ($case)" => [ + '/wiki/FooBar', + $options, $defaultProto, + "$srv/wiki/FooBar", + ]; } } } } - } - - public static function provideExpand(): Generator { - $modes = [ 'http', 'https' ]; - - foreach ( self::provideExpandCases() as [ $options, $protoDesc, $defaultProto ] ) { - $case = "default: $protoDesc"; - foreach ( $options as $key => $val ) { - $case .= ", $key: $val"; - } - - yield "No-op fully-qualified http URL ($case)" => [ - 'http://example.com', - $options, $defaultProto, - 'http://example.com', - ]; - yield "No-op fully-qualified https URL ($case)" => [ - 'https://example.com', - $options, $defaultProto, - 'https://example.com', - ]; - yield "No-op fully-qualified https URL with port ($case)" => [ - 'https://example.com:222', - $options, $defaultProto, - 'https://example.com:222', - ]; - yield "No-op fully-qualified https URL with standard port ($case)" => [ - 'https://example.com:443', - $options, $defaultProto, - 'https://example.com:443', - ]; - yield "No-op rootless path-only URL ($case)" => [ - "wiki/FooBar", - $options, $defaultProto, - 'wiki/FooBar', - ]; - - // Determine expected protocol - switch ( $protoDesc ) { - case 'protocol-relative': - $p = ''; - break; - case 'fallback': - case 'current': - $p = $options[UrlUtils::FALLBACK_PROTOCOL] . ':'; - break; - - case 'canonical': - $p = strtok( $options[UrlUtils::CANONICAL_SERVER], ':' ) . ':'; - break; - - case 'internal': - $p = strtok( $options[UrlUtils::INTERNAL_SERVER], ':' ) . ':'; - break; - - case 'http': - case 'https': - $p = "$protoDesc:"; - } - yield "Expand protocol-relative URL ($case)" => [ - '//wikipedia.org', - $options, $defaultProto, - "$p//wikipedia.org", - ]; - - // Determine expected server name - if ( $protoDesc === 'canonical' ) { - $srv = $options[UrlUtils::CANONICAL_SERVER]; - } elseif ( $protoDesc === 'internal' ) { - $srv = $options[UrlUtils::INTERNAL_SERVER]; - } elseif ( substr( $options[UrlUtils::SERVER], 0, 2 ) === '//' ) { - $srv = $p . $options[UrlUtils::SERVER]; - } else { - $srv = $options[UrlUtils::SERVER]; - } - - // Add a port only if it's not the default port, the server - // protocol is relative, and we're actually trying to produce an - // HTTPS URL - if ( $options[UrlUtils::HTTPS_PORT] !== 443 && - substr( $options[UrlUtils::SERVER], 0, 2 ) === '//' && - ( $defaultProto === PROTO_HTTPS || ( $defaultProto === PROTO_FALLBACK && - $options[UrlUtils::FALLBACK_PROTOCOL] === 'https' ) ) - ) { - $srv .= ':' . $options[UrlUtils::HTTPS_PORT]; - } - - yield "Expand path that starts with slash ($case)" => [ - '/wiki/FooBar', - $options, $defaultProto, - "$srv/wiki/FooBar", - ]; - } + // Non-standard https port + $optionsRel111 = [ + UrlUtils::SERVER => '//wiki.example.com', + UrlUtils::CANONICAL_SERVER => 'http://wiki.example.com', + UrlUtils::HTTPS_PORT => 111, + UrlUtils::FALLBACK_PROTOCOL => 'https', + ]; + yield "No-op foreign URL, ignore custom port config" => [ + 'https://foreign.example.com/foo', + $optionsRel111, PROTO_HTTPS, + 'https://foreign.example.com/foo', + ]; + yield "No-op foreign URL, preserve existing port" => [ + 'https://foreign.example.com:222/foo', + $optionsRel111, PROTO_HTTPS, + 'https://foreign.example.com:222/foo', + ]; + yield "Expand path with custom HTTPS port" => [ + '/foo', + $optionsRel111, PROTO_HTTPS, + 'https://wiki.example.com:111/foo', + ]; // Silly corner cases yield 'CanonicalServer with no protocol' => [ -- cgit v1.2.3