6.0.0-git
2019-04-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 2015-10-08 (1295 days ago)
Due
Updated 2016-02-11 (1169 days ago)
Assigned 2015-10-11 (1292 days ago)
Resolved 2016-02-11 (1169 days ago)
Milestone
Patch Yes

History
2016-02-11 15:08:31 Jan Schneider Assigned to Jan Schneider
State ⇒ Resolved
 
2016-01-14 13:54:30 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
2016-01-14 13:54:26 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
2015-10-12 11:42:07 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.

2015-10-12 11:20:12 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.

2015-10-12 05:44:53 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.
2015-10-11 23:02:51 Michael Rubinsky State ⇒ Feedback
Priority ⇒ 1. Low
 
2015-10-11 23:02:38 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.

2015-10-08 08:41:17 birnbacs (at) gmail (dot) com Comment #1
Type ⇒ Bug
State ⇒ Unconfirmed
Priority ⇒ 2. Medium
Summary ⇒ monthdayyear form element not saved in DB
Due ⇒ 2015-10-30
Queue ⇒ Horde Framework Packages
Milestone ⇒
Patch ⇒ Yes
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