6.0.0-beta1
7/8/25

[#14130] monthdayyear form element not saved in DB
Summary monthdayyear form element not saved in DB
Queue Horde Framework Packages
Type Bug
State Resolved
Priority 1. Low
Owners jan (at) horde (dot) org
Requester birnbacs (at) gmail (dot) com
Created 10/08/2015 (3561 days ago)
Due
Updated 02/11/2016 (3435 days ago)
Assigned 10/11/2015 (3558 days ago)
Resolved 02/11/2016 (3435 days ago)
Github Issue Link
Github Pull Request
Milestone
Patch Yes

History
02/11/2016 03:08:31 PM Jan Schneider Assigned to Jan Schneider
State ⇒ Resolved
 
01/14/2016 01:54:30 PM Git Commit Comment #7 Reply to this comment
Changes have been made in Git (master):

commit 75e9b5e3065dae60273c7920a83c804de3a64afc
Author: Jan Schneider <jan@horde.org>
Date:   Thu Jan 14 14:52:15 2016 +0100

     [jan] Allow any empty format specifiers for the monthdayyear 
field (Bug #14130).

  framework/Form/package.xml |    2 ++
  1 files changed, 2 insertions(+), 0 deletions(-)

http://github.com/horde/horde/commit/75e9b5e3065dae60273c7920a83c804de3a64afc
01/14/2016 01:54:26 PM Git Commit Comment #6 Reply to this comment
Changes have been made in Git (master):

commit 9f65a05d470ee9e2ca797ce26f09acb4dcef9b5c
Author: Jan Schneider <jan@horde.org>
Date:   Thu Jan 14 14:51:08 2016 +0100

     Loosen the the test for the date format (Bug #14130).

     Theoretically it might have been intended to explicitly pass an 
empty string (as opposed to a null value) as the date format. But this 
doesn't make sense because that would generate a form field that never 
has a result value. Beside that we already *do* catch an empty value, 
even if it's a strict null check.

  framework/Form/lib/Horde/Form/Type.php |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

http://github.com/horde/horde/commit/9f65a05d470ee9e2ca797ce26f09acb4dcef9b5c
10/12/2015 11:42:07 AM birnbacs (at) gmail (dot) com Comment #5 Reply to this comment
I agree, but if you are protecting against an "empty" value, you 
should explicitly check for this using empty(). This makes the 
code's intent much cleaner. Setting the equality check as you did 
can hide other issues (such as in this case where some code is 
passing a value that is seemingly incorrect) or make them harder to 
track down.
True.
An additional check for is_string() would probably be overkill.

10/12/2015 11:20:12 AM Michael Rubinsky Comment #4 Reply to this comment
The code in question should protect itself against not meaningful or 
erroneous parameters. It seems legitimate to not accept null as a 
format string as the null format never makes sense. The sames´goes 
for formats like an empty array, 0 and false.
I agree, but if you are protecting against an "empty" value, you 
should explicitly check for this using empty(). This makes the code's 
intent much cleaner. Setting the equality check as you did can hide 
other issues (such as in this case where some code is passing a value 
that is seemingly incorrect) or make them harder to track down.

10/12/2015 05:44:53 AM birnbacs (at) gmail (dot) com Comment #3 Reply to this comment
The code in question should protect itself against not meaningful or 
erroneous parameters. It seems legitimate to not accept null as a 
format string as the null format never makes sense. The sames´goes for 
formats like an empty array, 0 and false.

More errors may wait up the call stack but this also seems to be one.
This isn't really a correct fix. Changing == checks only for 
equivalence of value, not type. So a value of e.g., false or 0 will 
be considered equivalent to null in this case. Probably what is 
happening is there is some code further up the chain that is passing 
a false-ish value to the format_in parameter instead of null.
10/11/2015 11:02:51 PM Michael Rubinsky Priority ⇒ 1. Low
State ⇒ Feedback
 
10/11/2015 11:02:38 PM Michael Rubinsky Comment #2 Reply to this comment

This isn't really a correct fix. Changing == checks only for 
equivalence of value, not type. So a value of e.g., false or 0 will be 
considered equivalent to null in this case. Probably what is happening 
is there is some code further up the chain that is passing a false-ish 
value to the format_in parameter instead of null.

10/08/2015 08:41:17 AM birnbacs (at) gmail (dot) com Comment #1
Priority ⇒ 2. Medium
Type ⇒ Bug
Summary ⇒ monthdayyear form element not saved in DB
Due ⇒ 10/30/2015
Queue ⇒ Horde Framework Packages
Milestone ⇒
Patch ⇒ Yes
State ⇒ Unconfirmed
Reply to this comment
I created a ticket type in whups that uses a custom field of type 
monthdayyear but it wouldn't work, i.e. no matter what I entered the 
values were not written into the backend.

I tracked down the bug to the Form module of pear. In Form/Type.php 
the monthdayyear object has a method called _validateAndFormat(). It 
appears that the comparison in line 3021 of $this->_format_in with 
null is using the wrong operator (===). I changed it to == and the 
parameter gets saved to (and retrieved from) the database.

Here is the method in question (fixed version):

    /**
      * Validate/format a date submission.
      */
     function _validateAndFormat($value, &$var)
     {
         /* If any component is empty consider it a bad date and return the
          * default. */
         if ($this->emptyDateArray($value) == 1) {
             return $var->getDefault();
         } else {
             $date = $this->getDateOb($value);
             if ($this->_format_in == null) { // line 3021, changed 
comparison operator from === to ==
                 return $date->timestamp();
             } else {
                 return $date->strftime($this->_format_in);
             }
         }
     }

Saved Queries