Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

URLs without scheme eg. //example.com/home should be valid URLs in Piwik #6652

Closed
wants to merge 2 commits into from

Conversation

av2k
Copy link

@av2k av2k commented Nov 12, 2014

Make URL recognition more conformant with RFC3986 and RFC1738 network-path references (URI beginning with a doubleslash).

When used with WP-Piwik in a Wordpress installation that uses such network-path references like '//domain' as base URI, Piwik does't recognize such a URI as valid, consequently putting a "http://" in front of it, resulting in a 'http:////domain' pattern which confuses the WP-Piwik plugin, resulting in multiple rewrites and creations of new sites.

Make URL recognition to be more conformant with RFC3986 and RFC1738 network-path references (URI beginning with a doubleslash).

When used with WP-Piwik in a Wordpress installation that uses such network-path references as base URI, Piwik does't recognize such a URI as valid, consequently putting a "http:" in front of it which confuses the WP-Piwik plugin, resulting in multiple rewrites and creations of new sites.
@mnapoli
Copy link
Contributor

mnapoli commented Nov 12, 2014

Thanks, can you add any test to cover that?

Tests for this class are in UrlHelperTest here.

@mattab
Copy link
Member

mattab commented Nov 17, 2014

@av2k the new test is not passing:

1) Piwik\Tests\Unit\Core_UrlHelperTest::test_getHostFromUrl
Failed asserting that null matches expected 'localhost'.

@mattab mattab added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Nov 17, 2014
@mattab
Copy link
Member

mattab commented Nov 28, 2014

Hi @av2k ping

@mattab mattab added this to the Short term milestone Nov 28, 2014
@av2k
Copy link
Author

av2k commented Nov 28, 2014

Hello,

thanks for your ping. Unfortunately, I can't reproduce the issue.

The test method does this:

$this->assertEquals('localhost', UrlHelper::getHostFromUrl('http://localhost'));
$this->assertEquals('localhost', UrlHelper::getHostFromUrl('http://localhost/path'));
$this->assertEquals('localhost', UrlHelper::getHostFromUrl('localhost/path'));
$this->assertEquals('localhost', UrlHelper::getHostFromUrl('//localhost/path'));
$this->assertEquals('sub.localhost', UrlHelper::getHostFromUrl('sub.localhost/path'));
$this->assertEquals('sub.localhost', UrlHelper::getHostFromUrl('http://sub.localhost/path/?query=test'));

When I execute the method manually I get the expected results:

echo getHostFromUrl('http://localhost') . "\n";
echo getHostFromUrl('http://localhost/path') . "\n";
echo getHostFromUrl('localhost/path') . "\n";
echo getHostFromUrl('//localhost/path') . "\n";
echo getHostFromUrl('sub.localhost/path') . "\n";
echo getHostFromUrl('http://sub.localhost/path/?query=test') . "\n";

localhost
localhost
localhost
localhost
sub.localhost
sub.localhost

@mattab mattab closed this in 0c05115 Dec 2, 2014
@mattab mattab added duplicate For issues that already existed in our issue tracker and were reported previously. and removed duplicate For issues that already existed in our issue tracker and were reported previously. labels Dec 2, 2014
@mattab mattab changed the title Update UrlHelper.php URLs without scheme eg. //example.com/home should be valid URLs in Piwik Dec 2, 2014
mattab added a commit that referenced this pull request Dec 2, 2014
@mattab
Copy link
Member

mattab commented Dec 3, 2014

Hi @av2k It was actually expected because parse_url is buggy on 5.3 and it was fixed in 5.4.7 see dfe30d9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants