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

Do not access API methods in plugins directly so other plugins can extend/manipulate the API output #7105

Closed
tsteur opened this issue Jan 29, 2015 · 13 comments
Labels
answered For when a question was asked and we referred to forum or answered it. c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Milestone

Comments

@tsteur
Copy link
Member

tsteur commented Jan 29, 2015

Eg in Controllers, Widgets, API, ... see eg https://github.com/piwik/piwik/blob/2.11.0-b3/plugins/SEO/Widgets.php#L41

$dataTable = API::getInstance()->getRank($url);

Problem is it won't trigger our API.* events, eg API.$pluginName.$methodName.end. In #7103 a user asked to make it possible to specify another provider for whois.com. This could be done via a plugin by hooking to the above mentioned event, removing the whois.com entry and adding another entry. Just an example as there would be still a call to whois.com... Problem is that doing this would be only visible in the API output, not in the widget itself. If the widget would call the API via API\Request or API\Proxy it would be automatically displayed in the UI.

This makes our platform automatically more extensible. This can be even more useful when plugins try to access the API of another plugin. Also I would consider is a bad practice when a plugin tries to access a class of another plugin directly, especially for API.

Task of this issue could be to replace some API calls, see implications re performance, and check if there are any other downsides etc. One downside would be eg worse autocompletion / inspection as we won't know what type the API method returns.

@tsteur tsteur added Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change. c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. labels Jan 29, 2015
@tsteur tsteur added this to the Mid term milestone Jan 29, 2015
@mnapoli
Copy link
Contributor

mnapoli commented Feb 10, 2015

Wouldn't it rather make sense to make the plugin itself more extensible instead of overriding the HTTP output?

For example with SEO metrics it's easy to make the SEO stats source extensible.

@diosmosis
Copy link
Member

👍

@tsteur
Copy link
Member Author

tsteur commented Dec 3, 2015

There are approx 500 calls to update. This is probably not doable for this patch release. Highest value with least performance drop would be to fix at least the ones used in Controller.php so developers can tweak UI output etc

@tsteur
Copy link
Member Author

tsteur commented Dec 3, 2015

What I found a bit difficult is that we have to be careful when converting direct API calls to Request::processRequest as this one applies our default filter_limit=100 so I often had to specify a filter_limit=-1 and it's easy to forget this one.

It would be nice to have this at some point outside of Piwik\API\Request and instead a RequestBuilder or something where we can do something like ->call('API.method')->fetchAll() or instead maybe Request::fetchAll() and Request::fetchOne() etc as some syntax sugar on top.

On the other side using processRequest() can have some advantages as one could specify API URL parameters in URLs of Controllers. Eg if one specifies filter_offset=10 in UsersManager one would only see all users from 10 to ... Downside: This filter_offset would be applied to all API calls during this page request and it would most likely result in an error. It's actually rather not an advantage. I would set filter_limit=-1,filter_offset=0, ... hard coded in Request::processRequest but this would break API. We need a new class for this and do this there.

@tsteur
Copy link
Member Author

tsteur commented Dec 3, 2015

Another problem is if there's say a date, period or segment parameter in the URL present and you call eg Request::processRequest('API.get') the system would automatically apply date, period and segment from the URL even if this is maybe not wanted. In some cases it might be wanted though. It should be rather specifically set otherwise the shown results in the UI might be different depending on the current URL. Eg if there might be maybe sometimes a date parameter in the Admin area and sometimes not.

@mattab
Copy link
Member

mattab commented Dec 4, 2015

@tsteur Moving this out of 2.15.1 for now, let me know if you have an idea for the "next step" or "proper solution" to this issue

@mattab mattab removed this from the 2.15.1 milestone Dec 4, 2015
@tsteur
Copy link
Member Author

tsteur commented Dec 6, 2015

Let's move it into 3.0.0-beta for now. We need to provide new method / class that doesn't take any parameter from $_GET/$_POST for mentioned reasons but also for security to avoid passing API parameters via a "controller URL".

@tsteur tsteur added this to the 3.0.0-b1 milestone Dec 6, 2015
@tsteur
Copy link
Member Author

tsteur commented Dec 9, 2015

Another problem with calling API methods: Sometimes API methods call other API methods within the same class directly like here where deleteUser() calls userExists() directly https://github.com/piwik/piwik/blob/2.15.0/plugins/UsersManager/API.php#L562 . However, userExists is a public API as well so an event for this method userExists would not be triggered and the API would behave in a wrong way

@tsteur
Copy link
Member Author

tsteur commented Sep 13, 2016

I am seeing here > 600 usages of API directly in the codebase. I think this is a project that is not quite feasible / worth it right now. I suggest to instead do it over time when we refactor parts of the platform anyway. When we refactor parts of it, it is important to refactor calls to the same method across the code base. Eg If some SitesManager.addSite calls are replaced to use Request::processRequest, we need to refactor it in all places where we call SitesManager.addSite. Otherwise parts of it behaves differently. I will close this issue for now but feel free to reopen.

@tsteur tsteur closed this as completed Sep 13, 2016
@sgiehl
Copy link
Member

sgiehl commented Sep 13, 2016

Should we convert all calls to Request::processRequest? Or should we build something like you mentioned some comments above:

It would be nice to have this at some point outside of Piwik\API\Request and instead a RequestBuilder or something where we can do something like ->call('API.method')->fetchAll() or instead maybe Request::fetchAll() and Request::fetchOne() etc as some syntax sugar on top.

Do you think it would maybe worth to create a ticket for that, so it is easier to refactor the direct calls?

@tsteur
Copy link
Member Author

tsteur commented Sep 13, 2016

We should convert all calls to Request::processRequest ideally. Or $request->processRequest() so it's injectable.

It would be nice to have this at some point outside of Piwik\API\Request and instead a RequestBuilder or something where we can do something like ->call('API.method')->fetchAll() or instead maybe Request::fetchAll() and Request::fetchOne() etc as some syntax sugar on top.

I'm not sure what I was thinking here :) I think this would be more useful as a standalone lib for our users when they call the HTTP API. In our case however, we would not go via HTTP.

The problem is simply replacing the API calls with processRequest() is likely not done. As mentioned above:

On the other side using processRequest() can have some advantages as one could specify API URL parameters in URLs of Controllers. Eg if one specifies filter_offset=10 in UsersManager one would only see all users from 10 to ... Downside: This filter_offset would be applied to all API calls during this page request and it would most likely result in an error. It's actually rather not an advantage. I would set filter_limit=-1,filter_offset=0, ... hard coded in Request::processRequest but this would break API. We need a new class for this and do this there.

This makes it quite complicated as we could easily introduce side effects. Like an API or Controller method is called, that calls another API via processRequest() and then it may use parameters from the original request if not all parameters were specified in processRequest() as it currently falls back to $_GET,$_POSThere: https://github.com/piwik/piwik/blob/2.16.3-b2/core/API/Request.php#L87-L99 . And even if we always defined all API parameters inprocessRequest(array(...))then it becomes a problem when an API method adds a new parameter. We might be able to fix this behaviour inPiwik\API\Request::processRequest()by making sure it always defines a$defaultRequest`. This kind of can break things as well but at least would not have side effects anymore.

@mattab
Copy link
Member

mattab commented Sep 27, 2016

@tsteur this issue was closed, should it really be closed? maybe we should reopen and put in Mid term or so?

@tsteur
Copy link
Member Author

tsteur commented Sep 27, 2016

I don't think it is realistic to ever work on this issue. Instead we need to refactor it all the time over the next years. Always leave the code in a better place whenever we change it :)

@mattab mattab added the answered For when a question was asked and we referred to forum or answered it. label Sep 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
answered For when a question was asked and we referred to forum or answered it. c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Projects
None yet
Development

No branches or pull requests

5 participants