-
Notifications
You must be signed in to change notification settings - Fork 62
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
symfony/yaml should be an optional dependency #45
Comments
I do not see how you would do this since that decision is made at runtime. There is no way to know in advance if a dev is going to use Yaml, JSON or a Php array. The only solution to this would be to split the package into 3 packages, one for each config file type. Originally, I wanted to use the the PECL Yaml extension because it performs better than the Symfony package but it does not come by default. So, I had to add a check in the code but needed the Symfony/Yaml regardless as a fall back. I ended up using the Symfony/Yaml for simplicity purposes (and easier testing). Let me know if you had something else in mind for the JSON vs. Php vs. Yaml. Thanks for posting. |
Why not just remove the config loading altogether and just have the developer pass an array in? That way they can load the configuration however they want and you don't have to check every time on every request what format the user is using. |
but i as library user may know which types i support in my project |
@djmattyg007 One of the main requirements for that library was to load from a config file. You don't have to use it, like you said you can read directly from an array. I'm not sure what's bothering you. Is it the footprint? Again, I think it could be simplified but those loader classes are pretty thin. I don't think it impact performance too much. |
yep. i'm worried about footprint.if i use your library and do not support loading yaml files in my project there's just dangling dependency on also i can not just rm -rf vendor/symfony/yaml because you instanciate the class from there. could use class_exists(symfony/yaml) && new symfony/yaml() options? |
I'm not sure I'm getting this. If you do install Monolog, chances are you're not going to use 90% of the handlers, processors, or even the Registry, etc. However you have to pull the entire package. I want the ability to load from Yaml or JSON easily. It think it is nice to have that flexibility. I don't want to worry about parsing the JSON or Yaml, validate any file format or whatever. It is just part of the package and part of the reasons why the package was created. However, I do realize not everyone wants to use the file "auto-loading/parsing" kind of thing. So, I guess one option would be what I mentioned above, which is skipping most of the Config loader and load the config from a |
But Monolog does not have "require" dependencies to optional components, it has only: "require": {
"php": ">=5.3.0",
"psr/log": "~1.0"
}, I'm fine with auto-loading, currently it's just not possible to omit |
how i see it is as this:
this way if i have |
There are many reasons to not want all these extra dependencies lying around. It's just more code I have to worry about keeping up to date. It's an increased footprint should I ever decide to bundle up my project as a single distributable archive. It might conflict with other components I decide to add to my project later on, as version requirements differ. Not to mention, some of the symfony components bring in their PHP compatibility packages, which bring in even more packages, despite the fact that I'm using PHP 5.6 and clearly don't need them. I also think it's ridiculous that you'd check on every single request what format is being used for configuration. I'm in the process of developing an application that can see dozens of requests made each second. Every extra runtime check literally slow everything down. I ended up just forking the project and removing all the bloat: https://github.com/djmattyg007/monolog-cascade. Feel free to take anything you want from it. |
Given that this package supports configuration in JSON and PHP arrays as well as YAML, not everyone is going to want symfony/yaml. Therefore, it should be an optional dependency.
The text was updated successfully, but these errors were encountered: