search

ironcamel / Dancer-Plugin-DBIC

DBIx::Class Plugin for Dancer
1 stars 13 forks source link

Add Plack::Middleware::DBIC::QueryLog support #6

Open ilmari opened 11 years ago

ironcamel commented 11 years ago

@ilmari I really appreciate the work you put into this, but after thinking hard about this, I don't think I like the way it is implemented. It is adding complexity and doing it in a bad way. It is what I would describe as an example of "action at a distance". The behavior changes if sometime earlier a particular module was loaded or not. I don't like that. You may argue that you implemented it in this way because of the limitations of the interface of this plugin, and I am sympathetic to that. I am willing to consider re-architecting this module in order to support the kind of thing you are trying to do.

One approach is to provide a new function such as post_process_schema which would allow you to register functions that can modify the schema before it is returned.

We could also consider providing config options for this kind of thing. I know that it may confuse things, since top level config options are used to "name" databases, but we already make a special exception for connect_info.

Another approach is to just leave this module alone and put your logic in your app. This module allows you to register and retrieve schema objects. If you want to modify a schema object to add debugging to it, simply retrieve it and do so.

I am open to other suggestions as well.

ilmari commented 11 years ago

Fair enough. Catalyst has a trait for the DBIC model, but Dancer doesn't have that kind of architecture.

As for putting the debug logic in the app, that goes against the idea of debug middlewares, which should be able to work without changing the app. Making it a config option would be fine. There's no top-level reserved names, connect_info is per schema, so a use_plack_querylog (or some such) option should be too. And if it's enabled by config it could make sense to die if we're not running under plack or there's no query log in the env.

ironcamel commented 11 years ago

Yeah, I like that approach. Maybe the config should be called debug_plack_querylog or

debug:
    - plack_querylog

or

middleware:
    - 'Plack::Middleware::DBIC::QueryLog'

to make it more extendable.

ilmari commented 11 years ago

Well, the loading of the middleware is already configurable:

plack_middlewares:
  -
    - Plack::Middleware::DBIC::QueryLog

The configuration I'm talking about is whether Dancer::Plugin::DBIC should apply the querylog object provided by the middleware.

ironcamel commented 11 years ago

I understand what you are trying to do. I was just considering how to name the config option in such a way as to be able to support similar things like this in the future. It's not a big deal. By the way, I just released a new version of this plugin with 2 more keywords, resultset and rset. They are just syntax sugar to save you some typing.