Skip to content

Make DateTime mutable again#1

Open
eshagh wants to merge 1 commit into
JodliDev:masterfrom
eshagh:master
Open

Make DateTime mutable again#1
eshagh wants to merge 1 commit into
JodliDev:masterfrom
eshagh:master

Conversation

@eshagh

@eshagh eshagh commented Apr 23, 2022

Copy link
Copy Markdown

@JodliDev, I echo @skug67
(f79a801#commitcomment-60637522)
in thanking you for creating the fork, its the only working kolab
calendar + caldav I've found.

However, I was having some trouble with timezones with invitations
processed using your fork. When accepting an invitation, it would always
land on my calendar in UTC. I presumed this was due to vobject returning
DateTimeImmutable and the existing code expecting to be able to modify
variables of DateTime. You added OR operators to catch the instances of
DateTimeImmutable where the code previously checked for DateTime, which
got things moving, but didn't handle timezones correctly.

In each place you added an OR for DateTimeImmutable, I added an if for
DateTimeImmutable turning it back into a mutable object, essentially
restoring the previous behavior while also accomodating the new
DateTimeImmutable that vobject spits out.

I tried initially doing it only where necessary, but there were too many
moles to whack so I did them all, and confirm now that timezone handling
is working as expected. There is certainly a better way to do this, but
this works.

@JodliDev, I echo @skug67
(JodliDev@f79a801#commitcomment-60637522)
in thanking you for creating the fork, its the only working kolab
calendar + caldav I've found.

However, I was having some trouble with timezones with invitations
processed using your fork. When accepting an invitation, it would always
land on my calendar in UTC. I presumed this was due to vobject returning
DateTimeImmutable and the existing code expecting to be able to modify
variables of DateTime. You added OR operators to catch the instances of
DateTimeImmutable where the code previously checked for DateTime, which
got things moving, but didn't handle timezones correctly.

In each place you added an OR for DateTimeImmutable, I added an if for
DateTimeImmutable turning it back into a mutable object, essentially
restoring the previous behavior while also accomodating the new
DateTimeImmutable that vobject spits out.

I tried initially doing it only where necessary, but there were too many
moles to whack so I did them all, and confirm now that timezone handling
is working as expected. There is certainly a better way to do this, but
this works.
@eshagh

eshagh commented Apr 23, 2022

Copy link
Copy Markdown
Author

Should note, needs php 7.4 or greater.

@skug67

skug67 commented Apr 23, 2022

Copy link
Copy Markdown

Thanks for the timezone fix. And after some poking around I was able to find a very simple fix for the all-day event point as well -- lines 135-137 of calendar.php appear to be adding one day to the duration of an all-day event (for what purpose, I have no idea). If you comment them out, the duration appears correctly.

@skug67

skug67 commented Apr 24, 2022

Copy link
Copy Markdown

Another observed glitch -- if I create an event using this plugin and participants to it, roundcube will send out an invitation. But when the event syncs via caldav, the attendees disappear. At that point (or for any invite initially created with a different calendar client and synced to the caldav server) I can no longer add participants to the event.

No idea at all how to go about debugging/fixing this.....

JodliDev added a commit that referenced this pull request Apr 25, 2022
@JodliDev

JodliDev commented Apr 25, 2022

Copy link
Copy Markdown
Owner

Thank you very much for that find! :)

Unfortunately DateTime::createFromImmutable() is not available before php 7.3 so we cant really use it.
Also, the more we move away from the original project, the harder it is to incorporate changes on their end. So I tried to go through your changes and only replaced the immutable date were I thought it was necessary.
(see f96299d)

I also took the time to incorporate their latest changes into this fork - it seems like their github archive is dead unfortunately...

Can you test if it works now with the newest version?

@ skug67 
Which file exactly are you referring to?
I just realised that the original project had issues disabled (which got copied over to my fork). My apologies, I enabled them now. I think its best if we continue in a separate issue :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants