Ensure metadata is available if page url can't be found #16920
Conversation
sgiehl
added
not-in-changelog
| $subTable = reset($subTables); | ||
| $result->setAllTableMetadata($subTable->getAllTableMetadata()); | ||
| } | ||
| return $result; |
There was a problem hiding this comment.
Will this actually set the correct period though? If we query with a multi period, then this will set it to the first subperiod, so in the last7 example, it would be 7 days ago. Is this what callers of the API would expect? I think maybe the Rss formatter doesn't support multiperiod requests, and some other changes are needed?
There was a problem hiding this comment.
Actually no. That piece of code is executed for datatable maps inside a map. I'll remove some useless code so it gets more clear.
|
It's a bit clearer, but I'm still unsure as to whether the period we pick in the list of tables will be what is expected by callers. I also noticed that this code doesn't appear to be tested (there don't seem to be any tests that call getPageUrl/getOutlink/etc.). Do you think it would be a good idea to add an integration or system test to cover this behavior and maybe check the period that is returned? If it's there isn't one already, perhaps there could be a rss test for this as well? |
|
@diosmosis I've just added some RSS tests |
|
@sgiehl is there a test that requests a page URL that cannot be found (to trigger the bug this PR fixes)? |
|
@diosmosis I've just added a test that should throw an exception on 4.x-dev |
Description:
When sending a request like this:
{"module":"API","format":"rss","pageUrl":"\xxxxxxxxxxxx","idSite":"1","period":"day","date":"last7","token_auth":"XYZANONYMIZED","method":"Actions.getPageUrl","filter_limit":100}
the api method
Actions.getPageUrlcurrently might return a completely empty datatable, if the url can't be found. But the RSS renderer tries to use theperiodmetadata of the datatable, which does not exist, and fails.Simply returning the first subtables metadata for the empty table response solves that issue.
fixes #16918
Review