@sgiehl opened this Pull Request on April 29th 2018 Member

Goal pattern type and pattern regexes will now be validated in API when creating or updating goals.

refs #12683

@tsteur commented on April 30th 2018 Member

Looks good and worked for me locally. Few tests are failing though such as https://travis-ci.org/matomo-org/matomo/jobs/372795044

@sgiehl commented on April 30th 2018 Member

Ah. Empty pattern types need to be allowed

@diosmosis commented on April 30th 2018 Member

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?

@sgiehl commented on April 30th 2018 Member

Don't have a strong opinion on that, but not sure if it's something that should / needs to be reusible

@diosmosis commented on April 30th 2018 Member

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).

@tsteur commented on April 30th 2018 Member

I think in this code might not be needed as it really depends on the way the preg_match is executed later see this PR. If a developer doesn't surround the regex in / but for example a tilde, then the validation might not 100% work or if the developer doesn't do the str_replace('/', '\/') etc

@sgiehl commented on April 30th 2018 Member

I've done some changes to use validator classes instead of directly checking it in API.
@tsteur @diosmosis if it was better the way before, let me know and I'll remove the commit again ;-)

@diosmosis commented on April 30th 2018 Member

If a developer doesn't surround the regex in / but for example a tilde, then the validation might not 100% work or if the developer doesn't do the str_replace('/', '\/') etc

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 :)

@sgiehl commented on May 3rd 2018 Member

Anything left here, or is that ready to merge?

This Pull Request was closed on May 3rd 2018
Powered by GitHub Issue Mirror