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
Comments
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. |
👍 |
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 |
What I found a bit difficult is that we have to be careful when converting direct API calls to It would be nice to have this at some point outside of On the other side using |
Another problem is if there's say a |
@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 |
Let's move it into |
Another problem with calling API methods: Sometimes API methods call other API methods within the same class directly like here where |
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 |
Should we convert all calls to
Do you think it would maybe worth to create a ticket for that, so it is easier to refactor the direct calls? |
We should convert all calls to
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
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 |
@tsteur this issue was closed, should it really be closed? maybe we should reopen and put in |
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 :) |
Eg in Controllers, Widgets, API, ... see eg https://github.com/piwik/piwik/blob/2.11.0-b3/plugins/SEO/Widgets.php#L41
Problem is it won't trigger our
API.*
events, egAPI.$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 viaAPI\Request
orAPI\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.
The text was updated successfully, but these errors were encountered: