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.

Sign up for GitHub

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

Jump to bottom

SitesManager.getSitesIdFromSiteUrl should match both HTTP and HTTPS #8072

Merged
merged 4 commits into from Jun 11, 2015
Merged

SitesManager.getSitesIdFromSiteUrl should match both HTTP and HTTPS #8072

merged 4 commits into from Jun 11, 2015

Conversation

mattab
Copy link
Member

@mattab mattab commented Jun 9, 2015

fixes #8070

@mattab mattab added the Needs Review PRs that need a code review label Jun 9, 2015
@mattab mattab added this to the 2.14.0 milestone Jun 9, 2015
@mattab mattab added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. and removed Needs Review PRs that need a code review labels Jun 9, 2015
tsteur
tsteur reviewed Jun 9, 2015
View reviewed changes
plugins/SitesManager/API.php

// return all variations of the URL
return array(
"http://" . $hostname,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it on purpose that we only allow http/https? Eg in mobile apps there could be URLs like fb://profile/33138223345 see http://en.wikipedia.org/wiki/Mobile_deep_linking Probably people wouldn't enter such URLs but maybe they do? Maybe we could always include the original $url as well` Seems a bit restrictive otherwise

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could always include the original $url as well` Seems a bit restrictive otherwise

Good point. Actually it does not work yet in Piwik to create websites with URLs that have weird scheme. Right now it does prepend the proper scheme;

db urls

still I've added test case anyway to make sure getSitesIdFromSiteUrl works for those URLs.

@mattab mattab added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Jun 11, 2015
@mattab
Copy link
Member Author

mattab commented Jun 11, 2015

Code feedback applied, except moving protected method somewhere else. If tests will pass it should be good to merge

mattab pushed a commit that referenced this pull request Jun 11, 2015
Merge pull request #8072 from piwik/8070
7d53836 
SitesManager.getSitesIdFromSiteUrl should match both HTTP and HTTPS
@mattab mattab merged commit 7d53836 into master Jun 11, 2015
@mattab
Copy link
Member Author

mattab commented Jun 11, 2015

Merged it myself as it's needed to fix System tests on master (i've already changed the sub-module to matomo-org/matomo-log-analytics#77 )

@mnapoli mnapoli deleted the 8070 branch June 11, 2015 11:46
@mattab mattab added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Jun 11, 2015
@mattab mattab mentioned this pull request Jul 7, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
2.14.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mattab @tsteur