uap-php icon indicating copy to clipboard operation
uap-php copied to clipboard

autoload Reader classes from a custom folder

Open Xuphey opened this issue 5 years ago • 2 comments

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

Xuphey avatar Apr 10 '19 06:04 Xuphey

Um...any more reviews?

Xuphey avatar Apr 24 '19 08:04 Xuphey

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.

lstrojny avatar Apr 26 '19 13:04 lstrojny