Possible error in DBIx::Class::TimeStamp

[Oleg Kostyuk-2]

Hello guys,

I'm look at this part of DBIx/Class/TimeStamp.pm :

=====  cut =====
sub add_columns {
  my ($self, @cols) = @_;
  my @columns;

  while (my $col = shift @cols) {
      my $info = ref $cols[0] ? shift @cols : {};

      if ( delete $info->{set_on_create} ) {
          $info->{dynamic_default_on_create} = 'get_timestamp';
      }

      if ( delete $info->{set_on_update} ) {
          $info->{dynamic_default_on_update} = 'get_timestamp';  # !!! (1)

          if ( defined $info->{dynamic_default_on_create} and
               $info->{dynamic_default_on_create} eq 'get_timestamp'
           ) {
              $info->{dynamic_default_on_update} = 'get_timestamp'; # !!! (2)
          }
      }

      push @columns, $col => $info;
  }

  return $self->next::method(@columns);
}
=====  cut =====

And can't understood, why we need (2), if we already did exactly same thing in (1) ?
May be, there is some error?


As for me, following code have more sence:

=====  cut =====
      if ( delete $info->{set_on_update} ) {
          $info->{dynamic_default_on_update} = 'get_timestamp';

          unless ( defined $info->{dynamic_default_on_create} and
               $info->{dynamic_default_on_create} eq 'get_timestamp'
           ) {
              $info->{dynamic_default_on_create} = 'get_timestamp';
          }
      }
=====  cut =====


I'm using:

% pmvers DBIx::Class
0.08108

Could anyone explain me, what going on here?
Thanks in advance.

--
Sincerely yours,
Oleg Kostyuk (CUB-UANIC)

_______________________________________________
List: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/dbix-class
IRC: irc.perl.org#dbix-class
SVN: http://dev.catalyst.perl.org/repos/bast/DBIx-Class/
Searchable Archive: http://www.grokbase.com/group/dbix-class@...

[Oleg Kostyuk-2]

Any thoughts?

2009/7/31 Oleg Kostyuk <[hidden email]>:

> Hello guys,
>
> I'm look at this part of DBIx/Class/TimeStamp.pm :
>
> =====  cut =====
> sub add_columns {
>   my ($self, @cols) = @_;
>   my @columns;
>
>   while (my $col = shift @cols) {
>       my $info = ref $cols[0] ? shift @cols : {};
>
>       if ( delete $info->{set_on_create} ) {
>           $info->{dynamic_default_on_create} = 'get_timestamp';
>       }
>
>       if ( delete $info->{set_on_update} ) {
>           $info->{dynamic_default_on_update} = 'get_timestamp';  # !!! (1)
>
>           if ( defined $info->{dynamic_default_on_create} and
>                $info->{dynamic_default_on_create} eq 'get_timestamp'
>            ) {
>               $info->{dynamic_default_on_update} = 'get_timestamp'; # !!!
> (2)
>           }
>       }
>
>       push @columns, $col => $info;
>   }
>
>   return $self->next::method(@columns);
> }
> =====  cut =====
>
> And can't understood, why we need (2), if we already did exactly same thing
> in (1) ?
> May be, there is some error?
>
>
> As for me, following code have more sence:
>
> =====  cut =====
>       if ( delete $info->{set_on_update} ) {
>           $info->{dynamic_default_on_update} = 'get_timestamp';
>
>           unless ( defined $info->{dynamic_default_on_create} and
>                $info->{dynamic_default_on_create} eq 'get_timestamp'
>            ) {
>               $info->{dynamic_default_on_create} = 'get_timestamp';
>           }
>       }
> =====  cut =====
>
>
> I'm using:
>
> % pmvers DBIx::Class
> 0.08108
>
> Could anyone explain me, what going on here?
> Thanks in advance.
>

--
Sincerely yours,
Oleg Kostyuk (CUB-UANIC)

_______________________________________________
List: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/dbix-class
IRC: irc.perl.org#dbix-class
SVN: http://dev.catalyst.perl.org/repos/bast/DBIx-Class/
Searchable Archive: http://www.grokbase.com/group/dbix-class@...

[Wallace Reis]

On Fri, Jul 31, 2009 at 2:33 PM, Oleg Kostyuk<[hidden email]> wrote:

> As for me, following code have more sence:
>
> =====  cut =====
>       if ( delete $info->{set_on_update} ) {
>           $info->{dynamic_default_on_update} = 'get_timestamp';
>
>           unless ( defined $info->{dynamic_default_on_create} and
>                $info->{dynamic_default_on_create} eq 'get_timestamp'
>            ) {
>               $info->{dynamic_default_on_create} = 'get_timestamp';
>           }
>       }
> =====  cut =====

Hmm. It seems to be right, except that I'm not sure if
dynamic_default_*on_create* should depend of set_*on_update*.

--
     wallace reis/wreis         Catalyst and DBIx::Class consultancy with a clue
     Software Developer and a commit bit: http://shadowcat.co.uk/catalyst/
 Shadowcat Systems Limited
 http://www.shadowcat.co.uk     http://www.linkedin.com/in/wallacereis

_______________________________________________
List: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/dbix-class
IRC: irc.perl.org#dbix-class
SVN: http://dev.catalyst.perl.org/repos/bast/DBIx-Class/
Searchable Archive: http://www.grokbase.com/group/dbix-class@...

[Oleg Kostyuk-2]

2009/8/6 Wallace Reis <[hidden email]>:
> On Fri, Jul 31, 2009 at 2:33 PM, Oleg Kostyuk<[hidden email]> wrote:
>
> Hmm. It seems to be right, except that I'm not sure if
> dynamic_default_*on_create* should depend of set_*on_update*.
>

As for me, this is obviously: if you have both fields, and
dynamic_default_on_update configured, and dynamic_default_on_create
exists but not configured, then it will be configured by TimeStamp
too. I just tried to leave original code untouched as much possible.
Probably, this code will be more correct:

=====  cut =====
      if ( delete $info->{set_on_update} ) {
          $info->{dynamic_default_on_update} = 'get_timestamp';
          unless ( exists($info->{dynamic_default_on_create})
                  and defined($info->{dynamic_default_on_create})
           ) {
              $info->{dynamic_default_on_create} = 'get_timestamp';
          }
      }
=====  cut =====

Thanks for your attention.

--
Sincerely yours,
Oleg Kostyuk (CUB-UANIC)

_______________________________________________
List: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/dbix-class
IRC: irc.perl.org#dbix-class
SVN: http://dev.catalyst.perl.org/repos/bast/DBIx-Class/
Searchable Archive: http://www.grokbase.com/group/dbix-class@...

[Oleg Kostyuk-2]

Hello guys,

This thread seems sleeping...

I still think that there is errors in code (see my previous message).
Could someone of commiters confirm or refute this?
And if this error, will be patch included into next release?

Thanks

--
Sincerely yours,
Oleg Kostyuk (CUB-UANIC)

_______________________________________________
List: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/dbix-class
IRC: irc.perl.org#dbix-class
SVN: http://dev.catalyst.perl.org/repos/bast/DBIx-Class/
Searchable Archive: http://www.grokbase.com/group/dbix-class@...

[Oleg Kostyuk-2]

I spent some time, and changed my opinion.



Now I'm pretty sure that this is just redundant code, effectively
no-op, and it can be safely deleted. Patch attached as
DBIx-Class-TimeStamp1.patch. It's impossible to create additional test
which will show what is not working without patching, because this
redundant code doesn't break anything. All current test after patching
is passed smoothly.



There is another possibility also - I think that original author
wanted to write something different, and I include this as
DBIx-Class-TimeStamp2.patch.

This could mean something like: "if we have both set_on_update and
dynamic_default_on_create, and dynamic_default_on_create is not from
our implementation, then use this foreign implementation for
set_on_update also". But this is not very good imho, at least because
we can't have any guarantee that method which is suitable for
dynamic_default_on_create, will be also suitable for
dynamic_default_on_update.

>From the other hand, if someone write:
    t_updated => {
        data_type => 'datetime',
        is_nullable => 0,
        dynamic_default_on_create => "somefunc",
        set_on_update => 1,
    },
and not set dynamic_default_on_update explicitly, then why
DBIx::Class::TimeStamp should make such assumptions?

So, I'm sure that second patch could lead to undesirable side effects
and could change expected (current) behaviour, and so should not be
applied.



Also, in DBIx-Class-TimeStamp3.patch - updates to POD, which is not
related to my question about code. Feel free to correct my "true
Oxford's English" ;)

--
Sincerely yours,
Oleg Kostyuk (CUB-UANIC)

[DBIx-Class-TimeStamp1.patch]

---

Index: lib/DBIx/Class/TimeStamp.pm
===================================================================
--- lib/DBIx/Class/TimeStamp.pm (revision 7427)
+++ lib/DBIx/Class/TimeStamp.pm (working copy)
@@ -54,12 +54,6 @@
 
         if ( delete $info->{set_on_update} ) {
             $info->{dynamic_default_on_update} = 'get_timestamp';
-
-            if ( defined $info->{dynamic_default_on_create} and
-                 $info->{dynamic_default_on_create} eq 'get_timestamp'
-             ) {
-                $info->{dynamic_default_on_update} = 'get_timestamp';
-            }
         }
 
         push @columns, $col => $info;

---

[DBIx-Class-TimeStamp2.patch]

---

Index: lib/DBIx/Class/TimeStamp.pm
===================================================================
--- lib/DBIx/Class/TimeStamp.pm (revision 7427)
+++ lib/DBIx/Class/TimeStamp.pm (working copy)
@@ -56,9 +56,9 @@
             $info->{dynamic_default_on_update} = 'get_timestamp';
 
             if ( defined $info->{dynamic_default_on_create} and
-                 $info->{dynamic_default_on_create} eq 'get_timestamp'
+                 $info->{dynamic_default_on_create} ne 'get_timestamp'
              ) {
-                $info->{dynamic_default_on_update} = 'get_timestamp';
+                $info->{dynamic_default_on_update} = $info->{dynamic_default_on_create};
             }
         }
 

---

[DBIx-Class-TimeStamp3.patch]

---

Index: lib/DBIx/Class/TimeStamp.pm
===================================================================
--- lib/DBIx/Class/TimeStamp.pm (revision 7427)
+++ lib/DBIx/Class/TimeStamp.pm (working copy)
@@ -39,6 +39,25 @@
 This is effectively trigger emulation to get consistent behavior across
 databases that either implement them poorly or not at all.
 
+BE WARNED, that for C<set_on_update> this trigger emulation will work ONLY
+if row object already have some dirty columns. If row object don't have
+any dirty columns, then C<set_on_update> WILL NOT be triggered. If you want
+C<set_on_update> work always, even when you don't have any dirty columns,
+then you should also set C<always_update> to true value:
+
+ __PACKAGE__->add_columns(
+    ........
+    t_updated => {
+        data_type => 'datetime',
+        set_on_create => 1,
+        set_on_update => 1,
+        always_update => 1,
+    },
+    ........
+ );
+
+For more details about C<always_update> see L<DBIx::Class::DynamicDefault>
+
 =cut
 
 sub add_columns {

---

_______________________________________________
List: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/dbix-class
IRC: irc.perl.org#dbix-class
SVN: http://dev.catalyst.perl.org/repos/bast/DBIx-Class/
Searchable Archive: http://www.grokbase.com/group/dbix-class@...

[Oleg Kostyuk-2]

Hey, commiters - any news here?
What is current status, patch was applied or not?

--
Sincerely yours,
Oleg Kostyuk (CUB-UANIC)

_______________________________________________
List: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/dbix-class
IRC: irc.perl.org#dbix-class
SVN: http://dev.catalyst.perl.org/repos/bast/DBIx-Class/
Searchable Archive: http://www.grokbase.com/group/dbix-class@...

[fREW Schmidt]

Oleg, I am planning on applying it, but for some reason the tests fail in trunk, so I have to get that working before I patch it.  I assure you that you haven't been forgotten :-)

On Thu, Sep 3, 2009 at 4:48 AM, Oleg Kostyuk <[hidden email]> wrote:

Hey, commiters - any news here?
What is current status, patch was applied or not?

--
Sincerely yours,
Oleg Kostyuk (CUB-UANIC)

_______________________________________________
List: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/dbix-class
IRC: irc.perl.org#dbix-class
SVN: http://dev.catalyst.perl.org/repos/bast/DBIx-Class/
Searchable Archive: http://www.grokbase.com/group/dbix-class@...

--
fREW Schmidt
http://blog.afoolishmanifesto.com

_______________________________________________
List: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/dbix-class
IRC: irc.perl.org#dbix-class
SVN: http://dev.catalyst.perl.org/repos/bast/DBIx-Class/
Searchable Archive: http://www.grokbase.com/group/dbix-class@...

[Oleg Kostyuk-2]

Thanks :)

--
Sincerely yours,
Oleg Kostyuk (CUB-UANIC)

_______________________________________________
List: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/dbix-class
IRC: irc.perl.org#dbix-class
SVN: http://dev.catalyst.perl.org/repos/bast/DBIx-Class/
Searchable Archive: http://www.grokbase.com/group/dbix-class@...

[Oleg Kostyuk-2]

>From time of last writing, we have three (!) releases - 0.08110,
0.08111, 0.08112 - but patches is still not applied. Do we have some
reason for this?...

Thanks.

--
Sincerely yours,
Oleg Kostyuk (CUB-UANIC)

_______________________________________________
List: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/dbix-class
IRC: irc.perl.org#dbix-class
SVN: http://dev.catalyst.perl.org/repos/bast/DBIx-Class/
Searchable Archive: http://www.grokbase.com/group/dbix-class@...

[Brian Cassidy]

On Tue, Sep 22, 2009 at 2:12 PM, Oleg Kostyuk <[hidden email]> wrote:
> >From time of last writing, we have three (!) releases - 0.08110,
> 0.08111, 0.08112 - but patches is still not applied. Do we have some
> reason for this?...

Forgive me if I'm talking out of turn, but, weren't your patches for
DBIx::Class::TimeStamp ...which is shipped separately from
DBIx::Class.

--
Brian Cassidy ([hidden email])

_______________________________________________
List: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/dbix-class
IRC: irc.perl.org#dbix-class
SVN: http://dev.catalyst.perl.org/repos/bast/DBIx-Class/
Searchable Archive: http://www.grokbase.com/group/dbix-class@...

[Oleg Kostyuk-2]

Sure, DBIx::Class::TimeStamp shipped separately. But as I understood,
fREW Schmidt told about failing tests in DBIx::Class. I think so
because on time of writing my email with patches he confirmed at IRC
(not to me, but to Ralf - I just see this) that all tests in
DBIx::Class::TimeStamp is passed ok with my patches applied. And so,
now I'm told about releases of DBIx::Class itself - if it was
released, then all troubles was fixed, isn't? - but patches to
DBIx::Class::TimeStamp is still not committed.

2009/9/22 Brian Cassidy <[hidden email]>:
> On Tue, Sep 22, 2009 at 2:12 PM, Oleg Kostyuk <[hidden email]> wrote:
>> >From time of last writing, we have three (!) releases - 0.08110,
>> 0.08111, 0.08112 - but patches is still not applied. Do we have some
>> reason for this?...
>
> Forgive me if I'm talking out of turn, but, weren't your patches for
> DBIx::Class::TimeStamp ...which is shipped separately from
> DBIx::Class.

--
Sincerely yours,
Oleg Kostyuk (CUB-UANIC)

_______________________________________________
List: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/dbix-class
IRC: irc.perl.org#dbix-class
SVN: http://dev.catalyst.perl.org/repos/bast/DBIx-Class/
Searchable Archive: http://www.grokbase.com/group/dbix-class@...

[Oleg Kostyuk-2]

Any news about this, please?...

2009/9/22 Oleg Kostyuk <[hidden email]>:

> Sure, DBIx::Class::TimeStamp shipped separately. But as I understood,
> fREW Schmidt told about failing tests in DBIx::Class. I think so
> because on time of writing my email with patches he confirmed at IRC
> (not to me, but to Ralf - I just see this) that all tests in
> DBIx::Class::TimeStamp is passed ok with my patches applied. And so,
> now I'm told about releases of DBIx::Class itself - if it was
> released, then all troubles was fixed, isn't? - but patches to
> DBIx::Class::TimeStamp is still not committed.
>
> 2009/9/22 Brian Cassidy <[hidden email]>:
>> On Tue, Sep 22, 2009 at 2:12 PM, Oleg Kostyuk <[hidden email]> wrote:
>>> >From time of last writing, we have three (!) releases - 0.08110,
>>> 0.08111, 0.08112 - but patches is still not applied. Do we have some
>>> reason for this?...
>>
>> Forgive me if I'm talking out of turn, but, weren't your patches for
>> DBIx::Class::TimeStamp ...which is shipped separately from
>> DBIx::Class.
>
> --
> Sincerely yours,
> Oleg Kostyuk (CUB-UANIC)
>

--
Sincerely yours,
Oleg Kostyuk (CUB-UANIC)

_______________________________________________
List: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/dbix-class
IRC: irc.perl.org#dbix-class
SVN: http://dev.catalyst.perl.org/repos/bast/DBIx-Class/
Searchable Archive: http://www.grokbase.com/group/dbix-class@...