@diosmosis opened this Pull Request on June 17th 2017 Member

This PR is a Proof of concept, so should not be merged. I want to get these ideas vetted before moving forward.

Changes in this PR

  • Extracted the $idArchives cache in Archive.php to a separate class that is loaded through DI. This decouples the cache from the specific Archive instance that holds it. It also makes the cache last throughout a request.
  • Extract archive ID fetching, archive data fetching & archiving process initialization from Archive.php to a new class called ArchiveTableStore. I didn't want to spend too much time changing things if this wasn't deemed a good idea, so I just moved code, I didn't refactor existing code.
  • Extract Archive instance creation into a new ArchiveQueryFactory class. This required making the Archive class constructor public.

Changes that haven't been made yet

  • Adding the archive table insertion code in ArchiveProcessor (& whatever other classes) to ArchiveTableStore.
  • Extracting the archive initialization process from ArchiveTableStore somehow (haven't thought of how).
  • Creating an interface for ArchiveTableStore to implement (called RecordDataStore or something) to generalize & specify how "archive data" is stored.

These don't have to be done in a single PR.

Purpose

The purpose of these changes are to allow plugins to do the following things:

  1. Override Archive instance creation via the new factory class to handle, eg, new period types.
  2. Supply a custom ArchiveTableStore to Archive class so it's possible to bypass archiving entirely for certain requests.

Together this will allow re-using other plugins' API methods that serve reports w/ a custom period type and/or disabling the use of the archive tables (so the request always aggregates & doesn't store new data in the archive tables).

Other things it might enable

Eventually the use of a RecordDataStore interface might enable different backends being used for archive data.

@mattab commented on June 20th 2017 Owner

@diosmosis Thanks for the PR! The concepts exposed look good. We have a short discussion with @tsteur @sgiehl and thought it makes sense so far. :+1:

@diosmosis commented on July 27th 2017 Member

Closing, will open a new PR with smaller changes

This Pull Request was closed on July 27th 2017
Powered by GitHub Issue Mirror