search

indiehd / audio-manipulator

The Audio Manipulator component of the indieHD Framework for PHP
MIT License
3 stars 1 forks source link

Use configuration file to set binary paths #8

Closed cbj4074 closed 5 years ago

cbj4074 commented 5 years ago

For details, see:

https://github.com/indiehd/audio-manipulator/pull/7#discussion_r277656737

cbj4074 commented 5 years ago

To articulate my hesitation a bit more thoroughly than I did in the comment linked in the Description, my concern is as follows:

Suppose that something like the Symfony Config component is leveraged to implement local configuration files, which are committed to version-control, within this repository, whose intention is to abstract-away the too-specific aspects of various configuration variable names.

For example, rather than referencing getenv('MID3V2_BINARY') in code, we move to something more like config('mp3.tagger.binary'). The corresponding config file might reside in config/mp3.php, for example. That's well and good, and it adds a level of abstraction that would, in theory, make these variable references more resilient, but it presents a couple challenges, too.

  1. Increases coupling between this library and Symfony Config (or whichever config solution is employed).
  2. How does an end-user override the config variable values in a project that includes this library as a dependency? Obviously, the end-user should not modify any file in this library, i.e., we don't want the individual messing-around in vendor/indiehd/audio-manipulator/config, which means that this library would need to be packaged for specific frameworks, e.g., Laravel, because, otherwise, there is no simple and portable means by which to "publish the config files" from vendor/indiehd/audio-manipulator/config into the end-user's own project.

Ultimately, I'm wondering how much value a change like this would truly add.

poppabear8883 commented 5 years ago

Answer 1, its simple, this entire Library is already very tightly coupled to Symfony's framework and its components as its the framework you chose to build it on ...

Answer 2, you are absolutely right in this concern which it had not even crossed my mind in this manner. However, there are ways to "deploy/bundle" the configuration file(s) into the projects scope by creating a Composer Script Ref: https://stackoverflow.com/a/17122677

The Value, the value in doing this is simply scalability... what if down the road you decide that it would be nice if this option was configurable by the end user ? but now you have several hundred files that don't have the ability to make this a simple feature addition and requires 70% rewritten because you didn't consider this possibility up front . I know the primary goal right now is to get this in a stable state or dare i say MVP, but i also know that your next step is to create a Laravel Specific package that makes use of this package. In this case I didn't know if you should consider this as lets do this now, just in case kind of situation ?

Over Engineering ?, yes ?

Am i opposed to ignoring this, no, It might never be necessary to implement this just to go unused.

cbj4074 commented 5 years ago

True enough, regarding the coupling to Symfony. And I'm okay with that, given how useful the Service Container is.

Good point, re: the Composer script approach. That is certainly a viable, lightweight option.

At this stage, I propose that we move this to a future milestone. In fact, maybe we should create a milestone for Laravel Integration, because, as you suggested, that will be necessary in order to bring this library into the indiehd/web-api repository.

Off-the-cuff, it doesn't seem like Laravel integration and/or adding config file support would break backwards compatibility (as a whole, it feels like more of a "feature"), so my vote is to table this for now and move it down the road just a bit.