Logging implementation problems

[Tobion]

There are several problems:

1. Client should implement https://github.com/php-fig/log/blob/master/Psr/Log/LoggerAwareInterface.php since it has a setLogger method. I know this would make Psr/Log a required dependency. But that is what it is for. The current fallback logging mechanism is somewhat hacky.
2. https://github.com/ruflin/Elastica/blob/master/lib/Elastica/Log.php is not really needed IMO. Also it uses error_log even for debug messages which is not appropriate I guess. Why not use Monolog?
3. Throwing an exception in https://github.com/ruflin/Elastica/blob/master/lib/Elastica/Client.php#L610 is not really useful.
4. The logging message https://github.com/ruflin/Elastica/blob/master/lib/Elastica/Client.php#L620 "logging Request" is bad. And the additional information should be the actual query that was sent to elastic search like http://localhost:9200/_bulk (PUT) { json data}, instead of splitting it in "data", "path", "connection". This way one could easily reproduce the query in ES.
5. Elastica logs in the "info" level for requests logs. Should be "debug". This would also be consistent with doctrine for example.

[ChristianRiesen]

+1 to all of it

[ruflin]

Some comments from my side:

1. As you mentioned correctly, it would require Psr/Log dependency. On the one hand I'm a big fan of composer and it makes dependency management quite simple. But not everyone is using it and if there are required dependencies in Elastica it will make it more complicated to just get started with it. So I would like to keep it optional. I prefer to fix the "hacky" fallback logging.
2. Monolog is again a dependency. We should make it available as option to error_log
3. Not sure why not. If a dependency is not met, I think an exception is quite a good option. What would you recommend?
4. We had something very similar in the past. I actually prefer to have the full request in the log as it has more informations then just the data. The main problem here where I agree with you, is that it is not easily readable in the log. For this we have created this util function: https://github.com/ruflin/Elastica/blob/master/lib/Elastica/Util.php#L147
5. Agree. @ChristianRiesen is taking care of this.

My suggestion would be, that we start fixing number 2 and 5, and start discussing on the other 3.

[Tobion]

To

1. Yes it's probably NOT needed to implement the interface (symfony often does not do it as well). This way Psr\Log remains optional. So we can kick this point.
2. My point is that Elastica should not care about the Logging implementation. So IMO https://github.com/ruflin/Elastica/blob/master/lib/Elastica/Log.php should be removed, which also means the log option for the client should be removed. The idea is that Elastica only implements what it is designed for (client for ES). There are already several logging libraries out there, so that's not the task of Elastica. So ideally there is only the setLogger method and Elastica uses the logger when one is set. That's it. The idea using Monolog was only necessary when you would want to keep the log option in the client.
3. If you follow 2., then 3. would automatically be removed because it doesn't make sense anymore.
4. I don't see what extra information are currently exposed than when using my suggested format?
5. ok

[ruflin]

I like your suggestion for number 2 even though it will break BC. As long as not logger is set, logging doesn't happen.

For number 4 I agree that about 90% of the information is the same (connection object has some additional info). But in case we push the logging to an external library I would suggest we provide the library with the object and it is up to the log implementation how and what should be written to the log.

[Tobion]

All these problems have been fixed in #1069. and I've opened #1661 to remove the outdated code.