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
add: SitesManager getIdFromSite() #1261
Comments
vote: stick with convention and keep the view access check. |
In light of being told that URLs may have multiple IDs, this should be changed to return an array:
if ($id) { Piwik::checkUserHasViewAccess( $id ); } ``` would need to iterate over the IDs array on multiple matches and make sure each had access. |
I didn't say to remove the access check (of course!). My point was that this code is invalid because it will error out if one other websites (that you don't have access to) has the URL you are requesting. Only websites that you have permissions to view should be returned. oooa, the Piwik::checkUserHasViewAccess function also accepts an array of IDs did you test this code? I see it reads "Idsite" when the field name is idsite.. |
Instead of checkUserHasViewAccess ... can you use the array of ids returned from getSitesIdWithAtLeastViewAccess() in the WHERE clause? i.e., idsite IN (?) |
probably, even though we want to avoid this as it doesn't scale (in the use case you have 2000 websites) |
The alternative is a subquery on the access table, WHERE idsite IN (SELECT idsite FROM access WHERE user = ?) |
Idsite is capitalized because the select, I guess. Code does work as-is! |
AFAICS, comment:3 (comment:6 ?) and unit tests are still todo items. |
(In [2061]) fixes #1261 - SitesManager: add getSitesIdFromSiteUrl( $url ) to API; add unit test |
(In [2062]) refs #1261 - add unit tests for more complete test coverage |
For plugins/SitesManager/API.php, companion function to getSiteFromId()
This should search the database for a URL and return the ID of that site. This is extremely useful in keeping two databases synced (WPMU and Piwik in my case)
Matthieu says to remove the viewAccess check, but I'm only following convention from getSiteFromId(). Feel free to remove.
Sorry I don't have time to submit a proper patch.
The text was updated successfully, but these errors were encountered: