Fwd: [Dbix-class] Breakage on .127 -> .192 upgrade

[Peter Rabbitson]

On Fri, May 20, 2011 at 12:58:09PM +0100, Pedro Melo wrote:
> Hi,
> 
> On Fri, May 20, 2011 at 3:00 AM, Peter Rabbitson <rabbit+dbic at rabbit.us> wrote:
> > On Fri, May 20, 2011 at 11:52:18AM +1000, Toby Corkindale wrote:
> >> Hi,
> >> I don't know how I failed to pick this up during the testing period
> >> for the .19x versions.
> >>
> >> I have some code that fails on dbic .192, but works when rolled back to .127.
> >> The failure is due to the Result class defining some accessors with
> >> mk_classdata() which are referred to from within a sqlt_deploy_hook()
> >> method.
> >
> > Known embarrassing regression (I made an incorrect assumption and noone caught
> > it after the fact). Look forward to 0.08193 by Monday the latest.
> 
> Peter, I was one of the persons who reported this on IRC.
> 
> I said I was going to provide a test case and a patch but I can only
> work on non-work stuff on the weekends. I was planning on working on
> this on saturday but from your answer I assume that something is
> already in the pipeline.
> 
> I'll check the git repo for the latest commits as soon as
> git.shadowcat.co.uk is back up on DNS.
> 
> In the meantime, I reverted the commit like this for my personal use:
> 
> https://github.com/melo/dbix-class/commit/ffdb8e2d7e01cc2a8d3b920c43c2d67c6748b988

I think you missed the backlog in-channel earlier. I was actually hoping you
guys to take care of it :)

[19:51:46] <riba> oh ffs
[20:18:12] <riba> http://paste.scsys.co.uk/104045
[20:18:15] <riba> arcanez: ^^
[20:18:21] <riba> melo: ^^
[20:18:37] <riba> please revert this commit, we'll ship a release within a week
[20:18:59] * riba fucks off


> The important feature for me was making sure that sql_deploy_hook()
> chaining, using next::method or next::can, worked, so that I could
> have several components using it.
> 
> When I was looking at the code to figure why this had stop working, I
> noticed that to a component, the whole sql_deploy_hook() setup is very
> fragile. If a resultsource changes the sqlt_deploy_callback(), the
> sql_deploy_hook() chain will never be called.

Yes I mentioned this in-channel too:

[19:44:21] <riba> basically sqlt_deploy_hook is not the correct code-entry
[19:44:30] <riba> as in it is not even guaranteed to be called
[19:45:07] <riba> sqlt_deploy_callback() is the only thing dbic calls, which by default would call the default_sqlt_deploy_hook which in turn knows how to find the proper sqlt_deploy_hook on the source and call that
[19:45:59] <riba> melo: at every point in this chain things are set-able, hence the assumptions you make are far from valid (and I do agree the whole thing is extremely misdesigned)

> It would be better for component writers had a hook point that is
> always called. Given that it would be a new hook point we can make
> sure it has the proper signature. It would be called in
> DBIx::Class::ResultSource::_invoke_sqlt_deploy_hook as the last thing
> like this:
> 
>   my $class = $self->result_class;
>   $class->new_name_for_new_sql_deploy_hook($self, @_) if $class;
> 
> What do you think?

I personally do not like this, but mainly because I hate sqlt_deploy_hook
in general - it is not a deploy() hook. It is invoked from within
deployment_statements(), which makes the whole thing even more confusing and
fragile (i.e. if you do have a ddl-dir already, none of your hooks will ever
fire). Furthermore the choice of having the "hooks" live in result classes
is down the same alley as ResultSetManager. I would strongly recommend
discussing with amiri/mst their experience working with ResultSource (note
not Class, Source) componentns, as this is the correct, natural place for
such a hook.

Note I *am* open to discussion on your earlier proposal, but please understand
that what you are proposing is a last-resort architectural compromise, which
I really would like to avoid.

Cheers