Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] splitting up responsibilities in Archive.php #11800

Closed
wants to merge 13 commits into from
Closed

[RFC] splitting up responsibilities in Archive.php #11800

wants to merge 13 commits into from

Conversation

diosmosis
Copy link
Member

@diosmosis diosmosis commented Jun 17, 2017

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 mattab added this to the 3.1.0 milestone Jun 20, 2017
@mattab mattab added Needs Review PRs that need a code review Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Jun 20, 2017
@mattab
Copy link
Member

mattab commented Jun 20, 2017

@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. 👍

@diosmosis
Copy link
Member Author

Closing, will open a new PR with smaller changes

@diosmosis diosmosis closed this Jul 27, 2017
@mattab mattab removed this from the 3.2.0 milestone Sep 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants