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

Added a common validation method to base API class #22637

Closed
wants to merge 4 commits into from

Conversation

snake14
Copy link
Contributor

@snake14 snake14 commented Oct 3, 2024

Description:

There are multiple plugins that need the same validation method. Rather than duplicate code, we'd like to add it to the parent class.

Review

@snake14 snake14 added the Needs Review PRs that need a code review label Oct 3, 2024
@snake14 snake14 requested a review from a team October 3, 2024 05:26
* @internal
* @throws Exception If the provided values aren't valid
*/
public function checkLastMinutes(int $maxMinutes, int $lastMinutes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is the best way how to do it, but also don't have a suggestion how to do it differently. The method probably doesn't need to be here if we have a different place where we provide similar config validations (and this doesn't even check config), so perhaps this could be somewhere else and reused from there?
Also, most likely this doesn't need to be public, can be just protected if it's just used from extending classes.

I'd wait for @sgiehl's suggestion how to do this differently.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michalkleiner Fair enough. I debated about putting it in a validator class, but that seemed focused on configs and settings when this isn't necessarily. I nearly had it protected, but made it public for easy unit testing. I can change it if that's preferable.

Copy link
Member

@sgiehl sgiehl Oct 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole method could also be achieved with one line:

(new \Piwik\Validators\NumberRange(0, $maxMinutes))->validate($lastMinutes);

But I personally would put that validation into each API method. It makes sense to put more complex stuff into reusable methods, but if it's only one line like above, I can't see much value in it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I got so focused on following what was done in CrashAnalytics that I forgot to check if an existing validator would work. I'll use that. Thank you.

@snake14 snake14 closed this Oct 6, 2024
@snake14 snake14 deleted the PG-2921-add-general-validation-method branch October 6, 2024 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants