From d54c3257fe8e5f92c1d3e78f7fc77e949fb5a4a8 Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Wed, 13 Jun 2018 17:30:25 +0300 Subject: [PATCH] Unify path and URL rule resolver (#40) * More tests * Use the same logic * No need to align like that * Refactor for components * Make it responsible for all the Regex logic * Even more tests * Better naming --- class/class-widget-context.php | 62 ++++++++++++++------- phpcs.xml | 4 +- tests/php/WidgetContextUrlTest.php | 88 +++++++++++++++++++++++++----- 3 files changed, 119 insertions(+), 35 deletions(-) diff --git a/class/class-widget-context.php b/class/class-widget-context.php index cd7e7e6..3515089 100644 --- a/class/class-widget-context.php +++ b/class/class-widget-context.php @@ -428,7 +428,8 @@ function context_check_url( $check, $settings ) { } /** - * Return the path relative to the root of the hostname. + * Return the path relative to the root of the hostname. We always remove + * the leading and trailing slashes around the URI path. * * @param string $uri Current request URI. * @@ -442,7 +443,7 @@ public function get_request_path( $uri ) { ) ); - $path = ltrim( $parts['path'], '/' ); + $path = trim( $parts['path'], '/' ); if ( ! empty( $parts['query'] ) ) { $path .= '?' . $parts['query']; @@ -454,37 +455,58 @@ public function get_request_path( $uri ) { /** * Check if the current request matches path rules. * - * @param string $path Current request relative to the root of the hostname. - * @param string $patterns A list of path patterns seperated by new line. + * @param string $path Current request relative to the root of the hostname. + * @param string $rules A list of path patterns seperated by new line. * * @return bool */ - function match_path( $path, $patterns ) { - $patterns_safe = array(); - $rows = explode( "\n", $patterns ); + function match_path( $path, $rules ) { + $path_only = strtok( $path, '?' ); + $patterns = explode( "\n", $rules ); - foreach ( $rows as $pattern ) { - // Trim leading slashes and whitespace. - $pattern = ltrim( trim( $pattern ), '/' ); + foreach ( $patterns as &$pattern ) { + // Use the same logic for parsing the visibility rules. + $pattern = $this->get_request_path( trim( $pattern ) ); + } - // Escape regex chars since we only support wildcards. - $pattern = preg_quote( $pattern, '/' ); + // Remove empty patterns. + $patterns = array_filter( $patterns ); - // Enable wildcard checks - $pattern = str_replace( '\*', '.*', $pattern ); + // Match against the path with and without the query string. + if ( $this->path_matches_patterns( $path, $patterns ) || $this->path_matches_patterns( $path_only, $patterns ) ) { + return true; + } - $patterns_safe[] = $pattern; + return false; + } + + /** + * Check if a URI path matches a set of regex patterns. + * + * @param string $path Request URI. + * @param array $patterns A list of patterns. + * + * @return bool + */ + public function path_matches_patterns( $path, $patterns ) { + if ( empty( $patterns ) ) { + return false; } - // Remove empty patterns - $patterns_safe = array_filter( $patterns_safe ); + foreach ( $patterns as &$pattern ) { + // Escape regex chars since we only support wildcards. + $pattern = preg_quote( trim( $pattern ), '/' ); + + // Enable wildcard checks. + $pattern = str_replace( '\*', '.*', $pattern ); + } - $regexps = sprintf( + $regex = sprintf( '/^(%s)$/i', - implode( '|', $patterns_safe ) + implode( '|', $patterns ) ); - return (bool) preg_match( $regexps, $path ); + return (bool) preg_match( $regex, $path ); } diff --git a/phpcs.xml b/phpcs.xml index fac450e..bae502e 100644 --- a/phpcs.xml +++ b/phpcs.xml @@ -2,7 +2,9 @@ - + + + diff --git a/tests/php/WidgetContextUrlTest.php b/tests/php/WidgetContextUrlTest.php index d7a7f04..da35a7b 100644 --- a/tests/php/WidgetContextUrlTest.php +++ b/tests/php/WidgetContextUrlTest.php @@ -10,20 +10,20 @@ class WidgetContextTest extends TestCase { 'http://example.com' => '', 'http://example.com/' => '', 'http://example.com/page' => 'page', - 'http://example.com/page/' => 'page/', + 'http://example.com/page/' => 'page', 'http://example.com/?query=param' => '?query=param', - 'http://example.com:9000/page/subpage/?query=param' => 'page/subpage/?query=param', + 'http://example.com:9000/page/subpage/?query=param' => 'page/subpage?query=param', ); protected $map_relative = array( '' => '', '/' => '', '/page' => 'page', - 'page/' => 'page/', - '/page/' => 'page/', - '/page/sub-page/' => 'page/sub-page/', + 'page/' => 'page', + '/page/' => 'page', + '/page/sub-page/' => 'page/sub-page', '/page?query=string' => 'page?query=string', - '/page/?query=string' => 'page/?query=string', + '/page/?query=string' => 'page?query=string', ); public function __construct() { @@ -31,21 +31,81 @@ public function __construct() { } public function testUrlMatch() { - $this->assertTrue( $this->plugin->match_path( 'page', 'page' ), 'Simple direct' ); - $this->assertTrue( $this->plugin->match_path( 'page/', 'page/' ), 'Direct with trailing' ); + $this->assertTrue( + $this->plugin->match_path( 'page/subpage', 'page/subpage' ), + 'Exact path' + ); + + $this->assertTrue( + $this->plugin->match_path( 'page', 'page/' ), + 'Exact with trailing' + ); } public function testUrlWildcards() { - $this->assertTrue( $this->plugin->match_path( 'page/subpage', 'page/*' ) ); - $this->assertFalse( $this->plugin->match_path( 'another-page', 'page/*' ) ); + $this->assertTrue( + $this->plugin->match_path( 'page/subpage', 'page/*' ), + 'Wildcard for all sub-pages' + ); + + $this->assertTrue( + $this->plugin->match_path( 'page', 'page*' ), + 'Wildcard for all slugs with a pattern' + ); + + $this->assertTrue( + $this->plugin->match_path( 'parent-page/page-slug', '*/page-slug' ), + 'Wildcard for any parent' + ); + + $this->assertFalse( + $this->plugin->match_path( 'page', 'page/*' ), + 'Wildcard for children only' + ); + + $this->assertFalse( + $this->plugin->match_path( 'another-page', 'page/*' ), + 'Wildcard for a totally different page' + ); } public function testUrlQueryStrings() { - $this->assertTrue( $this->plugin->match_path( 'page/subpage?query=string', 'page/*' ) ); - $this->assertTrue( $this->plugin->match_path( 'campaigns/?cc=automotive', 'campaigns/*' ) ); - $this->assertTrue( $this->plugin->match_path( 'campaigns/?cc=automotive', 'campaigns/?cc=*' ) ); + $this->assertTrue( + $this->plugin->match_path( 'page/subpage/?query=string', 'page/*' ), + 'Wildcard for subpage and a query string' + ); + + $this->assertTrue( + $this->plugin->match_path( 'campaigns/?cc=automotive', 'campaigns/*' ), + 'Wildcard for everything' + ); + + $this->assertTrue( + $this->plugin->match_path( 'campaigns?cc=automotive', 'campaigns/?cc=*' ), + 'Wildcard for a specific query variable' + ); + + $this->assertFalse( + $this->plugin->match_path( 'campaigns-another-page', 'campaigns/*' ), + 'Path not matching the wildcard' + ); + } + + public function testUrlSpecial() { + $this->assertTrue( + $this->plugin->match_path( 'campaigns?cc=automotive', 'campaigns/' ), + 'Ignore query string because no rules use it' + ); + + $this->assertFalse( + $this->plugin->match_path( 'campaigns?cc=automotive', 'campaigns/?some=other' ), + 'Respect query string if differen used' + ); - $this->assertFalse( $this->plugin->match_path( 'campaigns/?cc=automotive', 'campaigns/' ) ); + $this->assertFalse( + $this->plugin->match_path( 'campaigns?cc=automotive', 'campaigns/?has=query' ), + 'Ignore query string because no rules use it' + ); } public function testPathResolverAbsolute() {