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

autoload Reader classes from a custom folder #54

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Xuphey
Copy link

@Xuphey Xuphey commented Apr 10, 2019

A solution to #53

Reads src/Util/Logfile/Custom/ for more log reader
The custom log reader needs to be in the same namespace as AbstractReader

src/Util/Logfile/Custom/NginxCustomLogFormatReader.php Outdated Show resolved Hide resolved
src/Util/Logfile/AbstractReader.php Outdated Show resolved Hide resolved
simplify finder lookup
@Xuphey
Copy link
Author

Xuphey commented Apr 24, 2019

Um...any more reviews?

@lstrojny
Copy link
Contributor

I agree, it’s indeed a problem to be unable to add more custom log readers, makes the command significantly less useful.

The solution I find problematic in principle, since it adds a convention based mechanism to add more readers and the process of adding more readers leads to IO, which I think we should avoid. Plus in detail the use of spl_autoload_register as a global side-effect when the AbstractReader is loaded is problematic, since the autoloaders function internally as a stack, so each time we load the file, we register one more loader.

So it’s clear that we want the extendability but not the current implementation. I think what makes sense anyway is to have an API to register custom readers. In order to do so, I think we should relieve the AbstractReader of its factory responsibilities and implement a ReaderFactory, that offers static method get(string $line): ReaderInterface. Additionally we can add ReaderFactory::register(string $readerClass). This is the basic building block.

Now it’s an integration problem between the command and the factory in the sense of how to we tell the factory "there are other readers to consider". Three solutions come to mind:

  1. Add a command line option --custom-reader=Class\Of\Custom\Reader
  2. Introduce a configuration file
  3. Use an auto prepend file and invoke the method
  1. is pretty clean but not terribly convenient from a user perspective
  2. is pretty clean but we need to start doing configuration file handling and that doesn’t sound like a lot of fun
  3. ugly and rather weird

Given that I would go with 1) for now as it still allows to venture into 2) if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants