waza-ari / monolog-mysql

MySQL Handler for Monolog, which allows to store log messages to a MySQL Table
MIT License
141 stars 85 forks source link

Add support for SQLite databases when initializing #8

Closed FoxxMD closed 8 years ago

FoxxMD commented 9 years ago

During database initialization the method by which column names are found is different for SQLite than other drivers. This commit introduces connection detection and uses a SQLite specific query to get column names.

waza-ari commented 9 years ago

I'm not sure if it is advisable to include support for SQLite, given the name of the package. The basic idea in Monolog is to have one handler per database. On the other hand, there is only a minor difference. If possible, I'd prefer a solution which does not require any SQLite specific extraction of column names. Maybe there is a way to extract them in a way which works in both SQLite and MySQL? Then the package would also be usable with SQLite without actually doing distinctions. What do you think?

FoxxMD commented 9 years ago

Ah you know you're totally right about not including SQLite -- I was so focused on making the package work for me I didn't even think about what its purpose was (or the name) :laughing: I needed this to work with SQLite as I use it for the DB provider when running tests, but during normal operation I use MySQL.

I have looked into using a generic approach but I do not think it is possible to achieve (the way you are doing it) without using at least some sqlite specific statements...

However, if you were to change the behavior of monolog-mysql to initially create database if not exists with additional fields and forego the feature of automatically removing/adding fields if the additionalFields array changes you could make it more generic since there would be no need to get column metadata.

And TBH this is how I thought it worked at first. From the README

$additionalFields simple array of additional database fields, which should be stored in the database. The columns are created automatically, and the fields can later be used in the extra context section of a record.

This makes me think that the database is created with the fields but it was a bit of a surprise to learn that if you initialize with missing fields on an existing database monolog-mysql will actually remove the column (and consequently any data that was stored in them). What if I pushed two separate handlers with different additionalFields because I only wanted to include a subset of information on one handler? When that handler is initialized it would remove the missing columns and all my data!

I ran into this problem and it took me some time to figure out what was happening because I was getting out of range and missing value errors from mysql seemingly at random. Turns out it was because columns were being removed/added because of the different handlers. I would prefer the table is created just once and if I later wanted to alter it I would do it myself rather than monolog-mysql doing it. (Which would have made debugging the mysql errors I was getting easier)

So actually that would need to be a different PR but it would solve the problem of keeping everything generic and also fix what is, IMO, an unintuitive feature.

waza-ari commented 8 years ago

Closing this pull request for now. See #11 for an discussion how to go with addionalFields and how this feature can be "fixed".