laminas / laminas-diagnostics

A set of components for performing diagnostic tests in PHP applications
https://docs.laminas.dev/laminas-diagnostics/
BSD 3-Clause "New" or "Revised" License
55 stars 32 forks source link

[RFC]: Add a ResultData object as Result data #59

Closed bram123 closed 8 months ago

bram123 commented 1 year ago

RFC

Q A
Proposed Version(s) 1.x.x
BC Break? Yes/No/Maybe

Goal

Provide a structured class to use when providing extra details about a executed check.

Background

I'm looking into using this package to generate an API health endpoint, for that we'd need more data besides the fact that a check was successful. In previous PRs I already added some data to some checks, but got a review point to see if we can make this more structured/typed.

The idea is to add a ResultData object so Checks can provide extra data in a set format.

Considerations

Big question is, are the current Check's data returns covered by BC? DiskUsage::check()->getData is now always an int, even though it's typed to mixed|null.

If we were to have a class available, these checks couldn't just switch over. Otherwise users of this package, who currently assume it's an int, will suddenly get something else.

Proposal(s)

Add a new ResultData object into Laminas\Diagnostics\Result, or Laminas\Diagnostics\Result\Data. Preferably not simply key + value, but:

Quick proposal:

$data = new ResultData();
$data->addDetail(uptime, 500, seconds);

return new Success("Connected successfully", $data);

Appendix

I got this as review point in a previous PR: https://github.com/laminas/laminas-diagnostics/pull/57#discussion_r1020012800. The PR was merged already (thanks for that), but I didn't want to 'forget about it'.

Ocramius commented 1 year ago

Big question is, are the current Check's data returns covered by BC?

I would say no until the types are actually refined.

In fact, I would say that Success<T> should be defined, where we declare T on each of the various Check implementations.

I don't think that a ResultData structure is needed there, if the type is well defined: then it would be covered by BC.