loading Helper components before IC::DateTime breaks datetime inflation for Perl v5.8

[SysPete]

With Perl v5.8 if Helper::Row components are loaded before InflateColumn::DateTime then the dt columns do not get inflated. Not tested with all Helpers and this was discovered by accident when trying to lower Perl version requirements for a CPAN distro.

This commit includes tests which demonstrate the failure with Perl 5.8: https://github.com/SysPete/DBIx-Class-Helpers/commit/4ebaf727b46e830534da17fa50d30748595f1ef5

[frioux]

I'm looking into this, but for what it's worth, if you swap the order of the components in your test result class the test passes on 5.8. If anything my guess is that this is a subtle bug in MRO::Compat.

[ribasushi]

Leaving myself a breadcrumb in the form of minimum reproduction steps. The problem is relatively gnarly and tough to reason about, and I don't have the braincycles currently to properly writeup what the SanityCheck for this particular case should look like. More at some point post 0.0829xx

~$ perl -e '

  use warnings;
  use strict;
  use Data::Dumper;

  # important that an actual require() takes place
  #
  my @result_class_lines;
  unshift @INC, sub {
    $_[1] eq "Foo/Schema/SomeResult.pm"
      ? sub {
          return 0 unless @result_class_lines;
          $_ = shift @result_class_lines;
          return 1;
        }
      : ()
  };

  @result_class_lines = split /\n+/, <<"EOR";

    package Foo::Schema::SomeResult;

    use base "DBIx::Class::Core";

    __PACKAGE__->load_components(qw(
      FilterColumn
      InflateColumn::DateTime
    ));

    __PACKAGE__->table("bogus");

    __PACKAGE__->add_columns( date => {
      data_type => "date"
    } );

    1;
EOR

  {
    package Foo::Schema;

    use base "DBIx::Class::Schema";

    # this is what fucks everything up,
    # particularly https://metacpan.org/source/RIBASUSHI/DBIx-Class-0.082840/lib/DBIx/Class/Schema.pm#L374
    # courtesy of https://github.com/dbsrgits/dbix-class/commit/e6efde04
    __PACKAGE__->load_classes("SomeResult");
  }

  # output will be different pre- and post OLD_MRO, sigh...
  warn Dumper(
    Foo::Schema::SomeResult->result_source_instance->column_info("date")
  );
'

[ribasushi]

This was reduced further to fail on any perl. Fix pending, but at this point it has nothing to do with ::Helpers

https://rt.cpan.org/Ticket/Display.html?id=93976#txn-1647825

Cheers

[frioux]

Awesome. Thanks! I'll leave this open as a breadcrumb for others till the fix is released.

sent from a rotary phone, pardon my brevity On Jul 12, 2016 10:57 AM, "Peter Rabbitson" notifications@github.com wrote:

This was reduced further to fail on any perl. Fix pending, but at this point it has nothing to do with ::Helpers

https://rt.cpan.org/Ticket/Display.html?id=93976#txn-1647825

Cheers

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/frioux/DBIx-Class-Helpers/issues/61#issuecomment-232127038, or mute the thread https://github.com/notifications/unsubscribe/AAAf4_BAOfNAuf_wO75p-2uVeJT_UmeDks5qU9V5gaJpZM4HULiy .

[ribasushi]

Argh, actually this is two issues in one. First is now resolved via a sanity check warning (not in git yet):

perl -Ilib -e '
  use warnings;
  use strict;

  use Devel::Dwarn;

  {
    package Some::BaseResult;
    use base "DBIx::Class::Core";

    # this works
    #__PACKAGE__->load_components(qw( InflateColumn::DateTime FilterColumn ));

    # this does not
    __PACKAGE__->load_components(qw( FilterColumn InflateColumn::DateTime ));
  }

  {
    package Some::Result;
    use base "Some::BaseResult";

    __PACKAGE__->table("sometable");

    __PACKAGE__->add_columns(
      somecolumn => { data_type => "datetime" },
    );
  }

  {
    package Some::Schema;
    use base "DBIx::Class::Schema";

    __PACKAGE__->register_class( someresult => "Some::Result" );
  }

  my $s = Some::Schema->connect(sub{});
 '

-e: Schema Some::Schema=HASH(0x2542508) failed the 'valid_c3_composition' sanity check: Class 'Some::Result' *WAS* using the 'dfs' MRO affecting the lookup order of the following method(s): delete(), register_column(), update(). You MUST add the following line to 'Some::Result' right after strict/warnings:
  use mro 'c3';

The original issues @SysPete reported is separate and in the pipeline as well. Almost there :/

[ribasushi]

FINALLY fixed: Original issue as reported by @SysPete addressed by https://github.com/dbsrgits/dbix-class/commit/1cf609901 The secondary issue (RT#93976): https://github.com/dbsrgits/dbix-class/commit/12e7015a#diff-2751ab15670aa2a17b874bc174263d24

Sorry it took so long folks, the wait was def. worth it.

[melo]

Hey, thanks @ribasushi!! Just got bitten by this.

Anyway, moved DWIM to last on load_components() works around this issue.

[melo]

One thing that still baffles me:

• Inflate::DateTime uses DBIx::Class as parent;
• Helper::Row::RelationshipDWIM uses DBIx::Class::Row as parent;
• InflateColumn::Serializer doesn't have a parent at all...

But all are used on a Result load_component() call.

Also: if you edit the current Helper::Row::RelationshipDWIM use parent to use DBIx::Class, this problem also goes away...

Should these helpers use parent DBIC::Row?

/me is confused now...

[frioux]

InflateColumn::DateTime should arguably use ::Row as parent, especially since InflateColumn does. Did you try fixing both the other InflateColumn components to (correctly) have their baseclass be ::Row?

[melo]

I haven't but I'll give it a try tomorrow

[ribasushi]

InflateColumn::DateTime should arguably use ::Row as parent, especially since InflateColumn does.

This is not entirely correct - in a C3 composition graph the intermediate relationships are not relevant. This is why the upcomign sanchecker never looks at this - it only makes sure the resulting order is correct.

Changing the preexisting ancestry map would break more than it will fix (and for the time being it will happen silently, as there is still one more tricky bit blocking the current codebase, almost there).

TLDR: @ISA's of parents is a red herring - don't look there.

[melo]

@ribasushi, ok. So should we use any parent at all? I don't see the need for it when I just need to place some functions in the call chain...

[ribasushi]

So should we use any parent at all?

I am afraid I do not understand the question, especially given the context of this ticket.

[melo]

Open DBIx::Class::Helper::Row::RelationshipDWIM code: it is very simple and has no dependencies.

If you change use parent 'DBIx::Class::Row'; to just use parent 'DBIx::Class'; this problem goes away.

If I remove the use parent completely, it also works. Given that it is such a simple code, that doesn't depend on any other component, do we really need to setup @ISA?

I ask because because frankly I don't see why this use parent is being used. I don't quite understand C3 inner workings, but this classes are simple some form of mix-in that is inserted in the call stack of some methods, nothing more.

I would like so see some guidance to DBIC extension creators about this. Do we really need the use parent, if our extensions don't depend on any other? I can see why InflateColumn::DateTime load_component() InflateColumn given that it is an extension, but simple classes like RelationshipDWIM maybe should just not have a parent.

Unless there is a reason to have it?

[frioux]

Pedro I have gone down this path before and the end is insanity. For better or worse the solution is to wait for ribas checking framework. It's complicated and inconsistent

sent from a rotary phone, pardon my brevity

On Aug 28, 2016 8:55 AM, "Pedro Melo" notifications@github.com wrote:

Open DBIx::Class::Helper::Row::RelationshipDWIM code: it is very simple and has no dependencies.

If you change use parent 'DBIx::Class::Row'; to just use parent 'DBIx::Class'; this problem goes away.

If I remove the use parent completely, it also works. Given that it is such a simple code, that doesn't depend on any other component, do we really need to setup @ISA https://github.com/ISA?

I ask because because frankly I don't see why this use parent is being used. I don't quite understand C3 inner workings, but this classes are simple some form of mix-in that is inserted in the call stack of some methods, nothing more.

I would like so see some guidance to DBIC extension creators about this. Do we really need the use parent, if our extensions don't depend on any other? I can see why InflateColumn::DateTime load_component() InflateColumn given that it is an extension, but simple classes like RelationshipDWIM maybe should just not have a parent.

Unless there is a reason to have it?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/frioux/DBIx-Class-Helpers/issues/61#issuecomment-242982179, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAf4-a3FsGedwixfDE2TKriswFaO4y1ks5qka-EgaJpZM4HULiy .

[melo]

I don't doubt @frioux, but I have a large DBIC codebase that I'm generaly happy with, that has some components done by us, that we just seem to cargo cult and stick a use parent 'DBIx::Class' because "it works"(tm) and I would rather have some understanding of this.

Nothing more.

[melo]

Basically, I just want to understand what is the correct way to write this extensions, so that I can depend on my codebase not being broken in the future when I update DBIC. That's my bottom line.

[melo]

@frioux this is what you wrote about it https://blog.afoolishmanifesto.com/posts/mros-and-you/

I get the logic. I would like to hear from @ribasushi though, what is the recommended fix here, one that works with the next DBIC...

Thanks,

[frioux]

Yeah I don't mind.

fREW Schmidt https://blog.afoolishmanifesto.com

[ribasushi]

I would like to hear from @ribasushi though, what is the recommended fix here, one that works with the next DBIC

@melo sorry for not replying earlier, I've been... distracted. The direct answer to your question is this. Because of how many moving parts there are in the entire stack, there isn't a "one size fits all" answer on how to structure intermediate classes.

Instead the feature I linked takes the other approach of saying well... your current method stack is ambiguous... please do fix it by doing the following in your top-most consumer class

Given the current limbo, your best answer at the moment is "check out dc7d8991, run your codebase against it, and observe STDERR"

Cheers!

[melo]

Thanks for the explanation (and work...), @ribasushi