News:

Want to get involved in developing SMF, then why not lend a hand on our github!

Main Menu

getHolidayRange in Subs-Calendar.php BUG and FIX

Started by SoLoGHoST, November 02, 2010, 03:38:14 PM

Previous topic - Next topic

SoLoGHoST

Ok, so getting the range of holidays between '2010-01-0' and '2011-12-31' Gives me all holidays (cept for the '0004' year holidays, only gives me '0004' year holidays 1 time when it is supposed to call them twice), than it makes the $holidays key where the YEAR is inserted = 2011 for ALL holidays returned.  This is a problem, as is needs to return the CORRECT year string within the key index for $holidays.

The current function is this:
// Get all holidays within the given time range.
function getHolidayRange($low_date, $high_date)
{
global $smcFunc;

// Get the lowest and highest dates for "all years".
if (substr($low_date, 0, 4) != substr($high_date, 0, 4))
$allyear_part = 'event_date BETWEEN {date:all_year_low} AND {date:all_year_dec}
OR event_date BETWEEN {date:all_year_jan} AND {date:all_year_high}';
else
$allyear_part = 'event_date BETWEEN {date:all_year_low} AND {date:all_year_high}';

// Find some holidays... ;).
$result = $smcFunc['db_query']('', '
SELECT event_date, YEAR(event_date) AS year, title
FROM {db_prefix}calendar_holidays
WHERE event_date BETWEEN {date:low_date} AND {date:high_date}
OR ' . $allyear_part,
array(
'low_date' => $low_date,
'high_date' => $high_date,
'all_year_low' => '0004' . substr($low_date, 4),
'all_year_high' => '0004' . substr($high_date, 4),
'all_year_jan' => '0004-01-01',
'all_year_dec' => '0004-12-31',
)
);
$holidays = array();
while ($row = $smcFunc['db_fetch_assoc']($result))
{
if (substr($low_date, 0, 4) != substr($high_date, 0, 4))
$event_year = substr($row['event_date'], 5) < substr($high_date, 5) ? substr($high_date, 0, 4) : substr($low_date, 0, 4);
else
$event_year = substr($low_date, 0, 4);

$holidays[$event_year . substr($row['event_date'], 4)][] = $row['title'];
}
$smcFunc['db_free_result']($result);

return $holidays;
}


SUGGESTED CHANGE in order to fix this problem:

// Get all holidays within the given time range.
function getHolidayRange($low_date, $high_date)
{
global $smcFunc;

$low_year = (int) substr($low_date, 0, 4);
$high_year = (int) substr($high_date, 0, 4);
$diff_year = $high_year - $low_year;

// Get the lowest and highest dates for "all years".
if ($low_year != $high_year)
$allyear_part = 'event_date BETWEEN {date:all_year_low} AND {date:all_year_dec}
OR event_date BETWEEN {date:all_year_jan} AND {date:all_year_high}';
else
$allyear_part = 'event_date BETWEEN {date:all_year_low} AND {date:all_year_high}';

// Find some holidays... ;).
$result = $smcFunc['db_query']('', '
SELECT event_date, YEAR(event_date) AS year, title
FROM {db_prefix}calendar_holidays
WHERE event_date BETWEEN {date:low_date} AND {date:high_date}
OR ' . $allyear_part,
array(
'low_date' => $low_date,
'high_date' => $high_date,
'all_year_low' => '0004' . substr($low_date, 4),
'all_year_high' => '0004' . substr($high_date, 4),
'all_year_jan' => '0004-01-01',
'all_year_dec' => '0004-12-31',
)
);
$holidays = array();
$event_year = $low_year;

// We need to assign the correct year for all years, no matter what the month is!
while ($row = $smcFunc['db_fetch_assoc']($result))
{
if ($low_year != $high_year)
{
// Need to SET all 0004 Holidays for each year since these are only called once!
if (substr($row['event_date'], 0, 4) == '0004')
{
// Add it to each year!
for ($i = 0; $i <= $diff_year; $i++)
$holidays[$event_year + $i . substr($row['event_date'], 4)][] = $row['title'];
}
else
$holidays[$row['event_date']][] = $row['title'];
}
else
$holidays[$event_year . substr($row['event_date'], 4)][] = $row['title'];
}
$smcFunc['db_free_result']($result);

return $holidays;
}


You can test this out and see what I mean with the following code:
global $sourcedir;

require_once($sourcedir . '/Subs-Calendar.php');

$holiday = array();

$holiday = getHolidayRange('2010-01-0', '2011-12-31');

ksort($holiday);

foreach($holiday as $date => $hday)
{
   // gets the title of the holiday!
   foreach($hday as $title)
       echo $title . '<br />';

  // gets the date of the holiday format Y-m-d!
  // e.g. 2010-12-25 = Christmas!
   echo $date . '<br /><br />';
}


The above code will give you all holidays for the year 2011 and 2010 (cept for the holidays with a substr(event_date, 0, 4) as they will only appear once for the year with the substr($high_date, 0, 4)), however they will ALL have the same year (2011) attached to them, this is using the current SMF getHolidayRange function, replacing this function with my function will give you all calendar holidays for every month of every year having the CORRECT year string attached to the keys of the $holiday array.

The problem is that although SMF's function will return the holidays between the range of $low_date and $high_date, the year gets overwritten to be the last year (substr($high_date, 0, 4)), within the index of $holidays array, my fixed function code will assign the CORRECT year string to each $holiday array's index value and also fix the problem of getting the year of 0004 holidays (which are CONSTANT holidays) to all years in the range.

Not sure if the getEventRange and/or getBirthdayRange functions have the same bug or not, haven't tested those functions, as I only needed to get Holidays, but I would recommend checking these functions as well.  Although, this isn't really a Major bug in SMF, it is misleading and still a BUG, because if you place a range in this month that exceeds the difference of more than 12 months, holidays returned will obtain the WRONG year value and there will be DUPLICATE/INVALID holidays returned for the SAME YEAR (substr($high_date, 0, 4))

Cheers :)

Norv

Thank you for the report. :)

I will try to look into it ASAP.
To-do lists are for deferral. The more things you write down the later they're done... until you have 100s of lists of things you don't do.

File a security report | Developers' Blog | Bug Tracker


Also known as Norv on D* | Norv N. on G+ | Norv on Github

SoLoGHoST

Sure thing, was quite a surprise to me stumbling onto this itsy bitsy bug!  Glad to be of some help :)

SoLoGHoST

Ok, I actually overlooked the database BETWEEN statements that currently only work for a difference of 1 year, so updated the BETWEEN db statement to work for more than 1 year:

// Get all holidays within the given time range.
function getHolidayRange($low_date, $high_date)
{
global $smcFunc;

$low_year = (int) substr($low_date, 0, 4);
$high_year = (int) substr($high_date, 0, 4);
$diff_year = $high_year - $low_year;

// Get the lowest and highest dates for "all years".
if ($low_year != $high_year)
{
if ($diff_year > 1)
// More than 1 year difference, we need them all.
$allyear_part = 'event_date BETWEEN {date:all_year_jan} AND {date:all_year_dec}';
else
// This would work for a difference of 1 year ONLY!
$allyear_part = 'event_date BETWEEN {date:all_year_low} AND {date:all_year_dec}
OR event_date BETWEEN {date:all_year_jan} AND {date:all_year_high}';
}
else
$allyear_part = 'event_date BETWEEN {date:all_year_low} AND {date:all_year_high}';

// Find some holidays... ;).
$result = $smcFunc['db_query']('', '
SELECT event_date, YEAR(event_date) AS year, title
FROM {db_prefix}calendar_holidays
WHERE event_date BETWEEN {date:low_date} AND {date:high_date}
OR ' . $allyear_part,
array(
'low_date' => $low_date,
'high_date' => $high_date,
'all_year_low' => '0004' . substr($low_date, 4),
'all_year_high' => '0004' . substr($high_date, 4),
'all_year_jan' => '0004-01-01',
'all_year_dec' => '0004-12-31',
)
);
$holidays = array();
$event_year = $low_year;

// We need to assign the correct year for all years, no matter what the month is!
while ($row = $smcFunc['db_fetch_assoc']($result))
{
if ($low_year != $high_year)
{
// Need to SET all 0004 Holidays for each year since these are only called once!
if (substr($row['event_date'], 0, 4) == '0004')
{
// Add it to each year!
for ($i = 0; $i <= $diff_year; $i++)
$holidays[$event_year + $i . substr($row['event_date'], 4)][] = $row['title'];
}
else
$holidays[$row['event_date']][] = $row['title'];
}
else
$holidays[$event_year . substr($row['event_date'], 4)][] = $row['title'];
}
$smcFunc['db_free_result']($result);

return $holidays;
}


Sorry bout that, because we are getting more than 1 year we'll need all 0004 holidays from 0004-01-01 to 0004-12-31

Cheers :)

Joshua Dickerson

Come work with me at Promenade Group



Need help? See the wiki. Want to help SMF? See the wiki!

Did you know you can help develop SMF? See us on Github.

How have you bettered the world today?

Arantor

This is one of those things that I'm not entirely convinced is a bug. It's a bug, but it's only a bug when it's used in a way different to how it was originally designed which means it's not really a bug. A car is a great vehicle until you try and drive it on a pond, then it won't be :P

Consider: this never manifests inside SMF normally. It only manifests because you have a range whereby the 'yearly' holidays are being pulled more than once - the code is really designed for the 3 months' range the calendar gets, which relies on the way annual holidays are stored essentially as an exception.

I guess I'm surprised anyone noticed, though I'm curious to know when you'd ever need more than a single year's worth of holidays at once outside the admin panel... I'm also concerned that this might actually have some side effects that need to be dealt with.

SoLoGHoST

This could be ignored since this is not technically a bug within SMF and how SMF uses that function.  But it's just a bug within the function itself, as it implies that it gets a holiday range between 2 specific dates, but really only gets a holiday range for 1 year.  But the bug is that it allows you to change the year part within the low and high dates, thus not giving you a proper return value when those years are different.

Arantor

Oh yes, I can see what the bug *is*, that part isn't a problem. My problem with this fix is, I'm not sure there aren't side effects with it. I haven't done enough testing on it or played with it enough yet to be comfortable about it working as 'expected'.

SoLoGHoST

No problem, I had to modify the Calendar Module with this change, which allows you to grab more than 3 months of holidays, in fact it allows you to grab any number of months (prev. or next from the current month).  I used this edit within the function and seems to work fine.  Module located here:  http://dream-portal.net/index.php?topic=833.0  I also changed other functions used for the SMF Calendar for better, overall, performance.

Arantor

Nothing says better performance like proper benchmarks showing the conditions under which improvement was obtained.

Advertisement: