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
Validate goal pattern and type in API #12788
Conversation
plugins/Goals/API.php
Outdated
@@ -218,6 +220,10 @@ private function checkPatternIsValid($patternType, $pattern, $matchAttribute) | |||
) { | |||
throw new Exception(Piwik::translate('Goals_ExceptionInvalidMatchingString', array("http:// or https://", "http://www.yourwebsite.com/newsletter/subscribed.html"))); | |||
} | |||
|
|||
if ($patternType == 'regex' && @preg_match($pattern, '') === false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it and it failed for me for rere(.*)
. I think this is because the /
fail. Ideally we would re-use same logic as here: https://github.com/matomo-org/matomo/blob/3.4.0/core/Tracker/GoalManager.php#L826-L836
to ensure it will work or not work during tracking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I've changed it :)
Looks good and worked for me locally. Few tests are failing though such as https://travis-ci.org/matomo-org/matomo/jobs/372795044 |
Ah. Empty pattern types need to be allowed |
Would it be a good idea to make these validations https://github.com/matomo-org/matomo/blob/3.x-dev/core/Validators/BaseValidator.php instances? |
Don't have a strong opinion on that, but not sure if it's something that should / needs to be reusible |
I'm thinking "is valid regex" could be reusable and "is one of values" could also be reusable (though this might already exist? not sure). |
I think in this code might not be needed as it really depends on the way the |
I've done some changes to use validator classes instead of directly checking it in API. |
This functionality could always be added when the need comes up (ie, a developer who uses the validation w/ a different delimiter will eventually notice the issue). But having the place to put this functionality (ie, a validator class) makes it possible. Otherwise these checks could get lost when someone eventually does add a regex validator and the same logic will be replicated in different places. That said, it's just validation, not a life or death issue. It's not like not using a validator will make matomo collapse with complexity :) |
Anything left here, or is that ready to merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Validate goal pattern and type * allow empty pattern type * Use validator classes to validate API params * tweak error message
Goal pattern type and pattern regexes will now be validated in API when creating or updating goals.
refs #12683