CRM-14137 Report_template.getrows api - allow retrieval of metadata at the top level

The fact that 'values' contains all data for every other api is 100% standardized right now and I'm not sure we should mess with that standard. Esp if we want to get rid of the wrapper around 'values' in v4.
What about a result like:
array(
'count' => x,
'is_error' => y,
'values' => array(
'header' => array(...),
'rows' => array(...),
),
)

hmm - I'm not adverse to that - although it seems less consistent in some ways - ie. we are then breaking the id-indexed array standard to meet the 'has no data other than values standard'. It will break json to the extent that it will return as 0 & 1 rather than header & rows due to the 'sequential' param expecting the values array to hold the 'real' values.

The response on the email thread was along the lines of 'it would be good to retrieve labels on other api too' & I took a look & found that pretty do-able.

Actually this thought process in this api & the email thread made me less keen on removing 'values'.

https://github.com/civicrm/civicrm-core/pull/2419

NB - this is what it looks like if we allow labels to be retrieved from any api

https://github.com/eileenmcnaughton/civicrm-core/commit/eede74dd411c25dde5de015b2eb4c3f5f396a64d

I think the 'labels' should not be part of the values array as Coleman suggests. Convention as Eileen points out is that the values is an ID-indexed array of results from the API call. The labels are not results per say, but rather metadata about the returned values, in the same way as 'is_error' or 'count'. They therefore should be returned one level up in the main structure.

Aren't we trying to cram two different apis in the same call?
we are get the instance attributes (title,label) and getting the rows of the instance. With good old chaining to the rescue:

civicrm_api3 ("report_template","get", array ("instance_id"=>42, "api.report_template.getrows"=>array())

and voila? I have no idea about the impact on the code/performance

Tangential, I'm more and more convinced that the proper call should have been
civicrm_api3 ("report","get", array ("id"=>42,...) instead of
civicrm_api3 ("report_template","get", array ("instance_id"=>42,...)

The problem with chaining is the performance. You have to run the whole thing report again to get the labels because they can be set at any stage during the rendering. The labels are not stored in the DB - they are calculated as part of running the report. Many reports do pretty heavy sql for good reason.

I guess we could back-up & make 'rows' a returnable on 'report_template', 'get' / report instance get. That would give us a logical place to put the results. The evils we perpetuate in that case are cobbling together 2 api in a way that ignore the fact they take very different params which hopefully (probably) don't overlap & adding another magic param (or magic return value).

Or we could create a new pseudo api entity 'report' - I'm not sure what you would key the results by though - as you would then key by the report template id OR the report instance id depending on whether a report instance was being retrieved or the base template was being retrieved. I guess you could expect the caller to know which it is

pushing all my issues to 4.4.5 so 4.4.4 isn't held up any longer

I'm not quite sure how to resolve this one as everyone disagrees.

Ok, I see your point about performance.

It's probably a similar issue we discussed a while ago about hooks: without having a context, we are forced to fetch the same thing several time.

Wouldn't it make sense to have the get aware there is a chained getrows? (and the other way around)?

That would allow the get to keep the rows and hand them down to the getrows.

Not sure how to get that clean without going OO. (ab)using globals or options? introducing a context?

Anyway, probably broader issue than 14137. something for api4?

X+

Yeah - it's possible. Personally it feels more cludgey than adding metadata at the top level. The main arguments against that seem to be 1) we don't want that for api v4 & 2) other api don't do it. WRT 2 - we also got the response 'but it would be nice if they did', - & it turned out it would be fairly easy to add as an optional return param. #1 - I think api v4 design is more relevant to api v4

Ok - we discussed this & agreed on the format per PR
https://github.com/civicrm/civicrm-core/pull/2458 & the ticket I have related

As I commented on CRM-14159 I think returning metadata at the top level is a good thing, and I like the format. For example in the website example you get:
$result = array(
'values' => array(...),
'metadata' => array(
'fields' => array(...)
),
)

That's cool. But now the report_template api I think needs a little cleanup to be consistent with all the others. Instead of returning title and labels at the top level along with values, I think we need:

$params = array('options' => 'metadata' => array('fields', 'title'))
$result = array(
'values' => array(...),
'metadata' => array(
'fields' => array(... field data arrays for consistency with other apis ...)
'title' => 'foo'
),
)

there is a separate 'fields' concept in reports which doesn't completely overlap with rows (& which I have some thoughts about altering) - so I think it's OK to leave the 'fields' space open for later