Duplicate event for recurring meeting instance exception #129
Comments
u01jmg3
changed the title
|
|
The one event is a single occurrence of a repeating event. In your translation of the iCal you'll notice the |
|
Gotcha. There is an issue of time zones being re-applied when populating However, when fixing this, it is the |
|
That's correct, I had it backward. The What is the fix for the timezones then? |
|
I believe this also may be happening with EXDATE |
|
Thanks - yes it all goes through |
|
I attempted a patch to try and return the correct timestamp. This worked on almost all my test files except one, where I believe the recurring event uses a different TZ format. I believe that the recurring events also may need to be parsed by timestamp with the correct conversions possibly with the same function. Here was my PHP code to display the entries date_default_timezone_set('America/New_York');
use ICal\ICal;
$ical = new ICal('ical.ics', array(
'defaultSpan' => 2, // Default value
'defaultWeekStart' => 'MO', // Default value
'skipRecurrence' => false, // Default value
'useTimeZoneWithRRules' => false, // Default value
));
$events = $ical->eventsFromRange($dtStartDay, $dtEndDay);
foreach ($events as $event) {
$dtStart = new \DateTime('now', new \DateTimeZone('America/New_York'));
$dtEnd = new \DateTime('now', new \DateTimeZone('America/New_York'));
$dtStart->setTimestamp((int) $ical->iCalDateToUnixTimestamp($event->dtstart));
$dtEnd->setTimestamp((int) $ical->iCalDateToUnixTimestamp($event->dtend));
echo sprintf(' <li>%s - %s : %s</li>' . PHP_EOL, $dtStart->format('r'), $dtEnd->format('r'), $event->summary);
}I believe I might have taken a different approach and not sure if this helps. |
|
Thanks for your efforts - your patch actually crashes my SourceTree but opening in an editor I get the gist of your change. Still working on an effective resolution. Hope to have something soon. |
|
The patch I did has been working for a few weeks now, well. The gist of the change is setting the TimeZone first then the time such that the conversions are done properly, inside the datetime object, as compared trying to figure out the time difference afterward and adjusting. You also have to explicitly set the TZ for the TimeDate object as I observed it seems to like to inherit this setting from elsewhere; unfortunately, I don't use TimeDate often enough to know best practice. |
|
Wow 16 days has flown by - apologies, I have been away. Will look at this again shortly |
|
Here is what I've been running. jasonheffner@db0049a#diff-50681920b8994c51dcfbb9a44850348f I have one meeting every other week that seems to break with recurrence I still need to look into. |
…ly the event time zone if explicitly requested
|
Finally made time to look at this. Using your diff I have tailored your code to provide a slightly different fix. The reason being I am dubious whether the time zone should even be applied when What I was finding before with your example was that the time zone was being re-applied to the See what you think.
|
|
Sorry for the delay after the holiday. I took a look at the code and tested it on a Month's worth of past days in April. It's closer but I do believe that you need to set the TimeZone each time for the TimeDate object based on the entry in the ical file. Currently, the code is working "most of the time" but I found a few days where it breaks since the TZ format changes and for some complex rules. It's working "relative" to your current TZ. For example, if you are working with everything in the same TZ as your default TZ set it would work fine, but if you are working with dates outside your current TZ, ie DST, then the timestamps returned are incorrect and skewed. If you look at the "timestamp" returned by |
|
Thanks - could you provide the test data which you are using? |
|
I shared some unedited versions with you to help troubleshoot. |
|
Thanks - this will help a lot. Can you point me to a few problematic events and what is wrong with them and what you would expect them to be? Perhaps a README in the repo with this info would give me a good starting point. |
|
Updated with a readme .. hopefully, illustrates the wrong timestamp set |
Enhance Event object to provide access to the ICal object and its functions
|
|
It's looking good .. the dupes are also gone in the branch which is an extra bonus. I was wondering why the need for the extra config parameter of If you wanted to force TZ wouldn't that be for overriding the one supplied in the iCal file already, meaning the option would be the opposite case? More trying to understand what you are going for. Perhaps it's just a difference in what we think the default handling should be. You are pulling the TZ out of iCal so thought you would want to use it by default. I tested in a few other dates and I noticed no dupes so it's working great. |
|
Thanks for taking the time to QA
This is more me being cautious as it has been tricky to nail down time zone logic in the parser with various other issues being raised. Once things are stable I will look to remove but for now I feel happier having it.
You're right - this could become confusing but as above I hope to phase out the parameter once the dust has settled and things become stable. I could rename the variable? Once you are happy I'll raise a PR and if you have time, could you review and then I will merge into |
|
Makes sense.. and it's easy to switch the default value later. I think the variable name works and I can set it to true for our parsing needs. It corrects all the parsing errors I've seen in the last month. This is the most complete ical parser I found for PHP. I've tried several, all with different problems. I like how you extended the class to include the TZ. That's the part I hadn't looked closely enough on how you were handling TZ within recurrences; I guess you weren't. I wasn't sure how best to implement that either and like your solution. |
Appreciate the compliment. Cast your eye over PR #142 and see what you think - made some final changes to refactor and improve the code I had already written but essentially it achieves the same thing and addresses the issues you raised. You may see some code that looks repetitive and not as neat as it could be - at this point I don't want to introduce any breaking changes to the parser but eventually I will bump to |
|
I went through the changes and only a few comments. I see what you did to the function so altered my code to match instead using the datetime object date_default_timezone_set('America/New_York');
use ICal\ICal;
$ical = new ICal('data.txt', array(
'defaultSpan' => 2, // Default value
'defaultWeekStart' => 'MO', // Default value
'skipRecurrence' => false, // Default value
'useTimeZoneWithRRules' => false, // Default value
));
$events = $ical->eventsFromRange($dtStartDay, $dtEndDay);
$ESTTZ = new DateTimeZone('America/New_York');
foreach ($events as $event) {
$dtStart = $ical->iCalDateToDateTime($event->dtstart, $forceTimeZone = true);
$dtEnd = $ical->iCalDateToDateTime($event->dtend, $forceTimeZone = true);
// Set TZ for display purposes
$dtStart->setTimezone($ESTTZ);
$dtEnd->setTimezone($ESTTZ);
}The setTimezone line #638 is really only needed for displaying the DateTime object in your preferred TZ at the end of the routine, as the times were entered into their corresponding TZs further up. I don't think you would need I saw quite a bit of repetitive code but never fault anyone for it. I'm a sysadmin so in most cases, it's more important for code to work correctly. I'm figuring at some point you'll go back and clean it up after the bugs and features are addressed. To note: I tried a lot of the PHP iCal parsers and this is the closest to correctly parsing all the ical features. Each one had it's own issues.. sabre/vobject was the most disappointing as I looked into that one first since GLPI uses it. The sorted |
This was added because when fixing the issue with the duplicates in your iCal, I found that the
I could add this as a new feature but as above I would have to keep the
👍 |
|
Ohh yeah, that makes sense! I was going to suggest " |
|
Unfortunately not, I still get some dates returned in the UTC TZ. I don't mind setting the TZ though and can switch to using |
|
|
See #142 |
|
Ditto! Thanks for your support. |
5.6.30America/New_YorkDescription of the Issue:
Duplicate meetings for recurring meetings. This happens on meetings that have single occurrences that have different times.
Steps to Reproduce:
Example iCal from Zimbra. On 20/04/2017 there should be 2x 1 hour meetings at 9:00 and
15:3014:00. Instead4x3x meetings are returned.TwoOne is a duplicate.The text was updated successfully, but these errors were encountered: