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

Jump to bottom

Allow API parameter type hints #19984

Closed
sgiehl opened this issue Nov 10, 2022 · 0 comments · Fixed by #20016
Closed

Allow API parameter type hints #19984

sgiehl opened this issue Nov 10, 2022 · 0 comments · Fixed by #20016
Assignees
@sgiehl
Labels
c: APIs For bugs and features in the Matomo HTTP and plugin APIs. c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. Stability For issues that make Matomo more stable and reliable to run for sys admins.
Milestone
5.0.0

Comments

@sgiehl
Copy link
Member

sgiehl commented Nov 10, 2022

Summary

Currently it is not possible to use type hints in API methods. As all parameters are parsed from the request, they are currently always either a string or an array. Adding type hints other then string or array would though currently end up in an error of mismatching types.

Allowing type hinting for API methods would help us solve a plenty of issues around incorrect parameter types. In the past we had a lot issues like that, where e.g. someone passed an array as period or date parameter (e.g. like #19222), causing the API to throw an uncaught exception, as it was used in a way, where an array couldn't be handled.

To implement that we could simply do some parameter validation in the API proxy, based on the type hint we can receive using reflection. Passing that type to Common::getRequestVar, which we use, to receive all parameter values should do the trick.
Needs to be changed somewhere around here:

matomo/core/API/Proxy.php

Lines 409 to 441 in a01eea3

private function getRequestParametersArray($requiredParameters, $parametersRequest)
{
$finalParameters = array();
foreach ($requiredParameters as $name => $defaultValue) {
try {
if ($defaultValue instanceof NoDefaultValue) {
$requestValue = Common::getRequestVar($name, null, null, $parametersRequest);
} else {
try {
if ($name == 'segment' && !empty($parametersRequest['segment'])) {
// segment parameter is an exception: we do not want to sanitize user input or it would break the segment encoding
$requestValue = ($parametersRequest['segment']);
} else {
$requestValue = Common::getRequestVar($name, $defaultValue, null, $parametersRequest);
}
} catch (Exception $e) {
// Special case: empty parameter in the URL, should return the empty string
if (isset($parametersRequest[$name])
&& $parametersRequest[$name] === ''
) {
$requestValue = '';
} else {
$requestValue = $defaultValue;
}
}
}
} catch (Exception $e) {
throw new Exception(Piwik::translate('General_PleaseSpecifyValue', array($name)));
}
$finalParameters[$name] = $requestValue;
}
return $finalParameters;
}

Note: This issue is not about adding type hints to all our API methods. For now we should only provide the possibility, so we can add it when already working on certain APIs later.

@mattab I've discussed this improvement today with @tsteur and @justinvelluppillai
It would be awesome to include this in Matomo 5. We could in theory also do it in a later release, but if we need to add a type hint in a plugin then, we would need to increase the minimum required version, to the version where this got implemented.

I already had a look at the code, and it should only take around 2 hours to implement that.
@sgiehl sgiehl added c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. c: APIs For bugs and features in the Matomo HTTP and plugin APIs. Stability For issues that make Matomo more stable and reliable to run for sys admins. labels Nov 10, 2022
@sgiehl sgiehl added this to the For Prioritization milestone Nov 10, 2022
@mattab mattab modified the milestones: For Prioritization, 5.0.0 Nov 14, 2022
@sgiehl sgiehl self-assigned this Nov 15, 2022
@sgiehl sgiehl mentioned this issue Nov 15, 2022
Merged
11 tasks
@sgiehl sgiehl closed this as completed Dec 5, 2022
@sgiehl sgiehl added the Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. label May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sgiehl sgiehl

Labels
c: APIs For bugs and features in the Matomo HTTP and plugin APIs. c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. Stability For issues that make Matomo more stable and reliable to run for sys admins.
Projects
None yet
Milestone
5.0.0
Development

Successfully merging a pull request may close this issue.

Make it possible to use type hinted parameters for API methods matomo-org/matomo
2 participants
@mattab @sgiehl