Problem/Motivation
In OrangeDamQueueDataManager::queueUpdatedItems(), the method checks the item count returned by OrangeDamApi::getContent() in order to determine whether to update the "check for updated content since" state.
There are two issues:
1) The returned count comparison strictly checks against FALSE, even though the method never returns FALSE. The intent here seems to be that if something went wrong during the API calls, to bounce out of OrangeDamQueueDataManager::queueUpdatedItems() before recording an updated "check for updated content since" state. The fact that getContent() returns no FALSE means that the "check for updated content since" state be updated if API failure occur, meaning that changes during that time period would be forever lost from retrieval, since the next check would only check for items changed since the new time stamp.
2) This relates to the second issue, which is that getContent() throws exceptions of various kinds, but also seems to catch some exceptions and silence them, depending on the kind of exception. This means that getContent() might fail silently under some circumstances, which allows queueUpdatedItems() to continue without knowing that an error occurred.
Proposed resolution
It seems the solution is multi-part, and requires choosing one approach: Use exceptions consistently, to fail under _all_ possible failures before the update to the "check for updated content since" state is set, or handle _all_ the exceptions and change the method signature to always return FALSE in the event of any exceptions. Right now, we have half-implementations of each approach.
Making getContent() return int|FALSE reduces clarity of the code by mixing the return type, so I am inclined to fix the above by ensuring that getContent() will fully fail and therefore never trigger run the code in queueUpdatedItems() where the timestamp is updated.
While we are at it, we might want to rename getContent() to queueContent() since that is actually what the method does. The name getContent implies that the return value should be the content. Changing it to queueContent() indicates an operation, and the return value would simply be the count of items queued.
try {
$count = OrangeDamApi::queueContent(); // Change getContent(): int to queueContent(): int
// Update time since timestamp
}
catch (Exception $e) {
// Don't update timestamp and just set $count to 0.
$count = 0;
}
return $count;
Alternatively, we could change the signature of queueUpdatedItems() to return bool TRUE on success and FALSE and failure, and handle all the exceptions. But then we would need to create another method to count queued items, since code elsewhere wants a count. So queueUpdatedContent() would like something like:
$success = OrangeDamApi::queueContent(); // Change getContent(): int to queueContent(): bool.
if ($success) {
// Update time since timestamp
}
$count = OrangeDamApi::getQueuedContentCount(); // New method returning int.
return $count;
There are two calls to OrangeDamApi::getContent() within OrangeDamQueueDataManager. One checks the return value for FALSE while the other simply assumes integer output and increments.
It looks like path forward is to ensure that any failures in getContent throw exceptions, so that any calling code can rely on success, even if the number of items is 0.