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

symfony/yaml should be an optional dependency #45

Open
djmattyg007 opened this issue Feb 9, 2016 · 9 comments
Open

symfony/yaml should be an optional dependency #45

djmattyg007 opened this issue Feb 9, 2016 · 9 comments

Comments

@djmattyg007
Copy link

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.

@rantonmattei
Copy link
Contributor

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.

@djmattyg007
Copy link
Author

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.

@glensc
Copy link
Contributor

glensc commented Feb 17, 2016

but i as library user may know which types i support in my project

@rantonmattei
Copy link
Contributor

@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 do agree though that the config reading/parsing is a bit heavy/over-engineered and could certainly be simplified. One way to get around this is to have additional loading functions in the Cascade class to load directly from a known file type. Feel free to shoot a PR.

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.

@glensc
Copy link
Contributor

glensc commented Feb 17, 2016

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 symfony/yaml

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?

@rantonmattei
Copy link
Contributor

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 public static function arrayConfig($array) function from Cascade.

@glensc
Copy link
Contributor

glensc commented Feb 17, 2016

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 symfony/yaml (rm -rf vendor/symfony/yaml)

@glensc
Copy link
Contributor

glensc commented Feb 17, 2016

how i see it is as this:

  • Yaml::supports should check if YamlParser class is available: if (!class_exists('YamlParser')) return false (may need full class name here, the use afaik is just syntactic sugar)
  • "require" of "symfony/yaml" should be moved to "suggests" in monolog-cascade
  • "require-dev" include "symfony/yaml" so you can run unit tests in monolog-cascade (require-dev is not propagated to dependant projects, only applies to root project)

this way if i have "symfony/yaml" in my own project composer, yaml support is present, otherwise not (as class is not present in autoloader)

@djmattyg007
Copy link
Author

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.

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

No branches or pull requests

3 participants