Astrotomic / laravel-monolog-config

This package provides a simple way to configure monolog in laravel.
MIT License
10 stars 3 forks source link

Using objects/closures in the config file #7

Open ye7iaserag opened 6 years ago

ye7iaserag commented 6 years ago

The config file needs to be updated to not include any objects use only class names To repeat try the Laravel config:cache command, the generated cached configuration will always throw a fatal error Please update the bundle Thanks

ye7iaserag commented 6 years ago

The exact lines are new LineFormatter() new Swift_Message('Laravel Log')

only class names should be used in configurations, example: LineFormatter::class Swift_Message::class

Gummibeer commented 6 years ago

The problem ist that some of these classes need parameters on construct. Do you have an idea how to solve it?

Gummibeer commented 6 years ago

I could think about a solution like:

'message' => [
    'class' => Swift_Message::class,
    'args' => ['Laravel Log'],
]

This could be used by http://php.net/manual/de/reflectionclass.newinstanceargs.php to get a new instance of the given class including parameters and it is serializable.

ye7iaserag commented 6 years ago

Yes that's exactly what was recommended by laravel's config files guidelines

ye7iaserag commented 6 years ago

Deserializing big objects from the config files is costly process while bootstrapping the framework, that's why they went against supporting closures in config files

Gummibeer commented 6 years ago

Ok, I will Update it tomorrow and give or a test.

Gummibeer commented 6 years ago

@ye7iaserag it's now in dev-master - implemented with https://github.com/Astrotomic/laravel-monolog-config/commit/e69fcbc35a7ebd46efd3d5aff80023a6ca6c170b Would be nice if you can give it a try, before I release it as v2.0 cause it's a breaking change. 😃

ye7iaserag commented 6 years ago

Will test is today and update you, but from what i can see that was exactly what was needed Good job :)

Gummibeer commented 6 years ago

@ye7iaserag any update?