Conversation
9d83b9f to
2cba151
Compare
88c8f76 to
864e40f
Compare
|
@georgehrke I'd say for now just write a small abstraction class yourself to solve the extra check issue. |
|
Most likely nothing for 14 -> moving to 15. |
b072922 to
c0868f8
Compare
fb7c1f4 to
1b17e2f
Compare
| if ($calendarObjectsTable->hasIndex('calobjects_index')) { | ||
| $calendarObjectsTable->dropIndex('calobjects_index'); | ||
| } | ||
| $calendarObjectsTable->addUniqueIndex(['calendarid', 'calendartype', 'uri'], 'calobjects_index'); |
There was a problem hiding this comment.
@nickvergessen Will building new indices be an issue? Should it be outsourced to a migration step?
There was a problem hiding this comment.
I think for this it is fine. Should be relatively fast.
However, a step first to see if there are duplicated might be needed.
As else it will just explode
There was a problem hiding this comment.
It removes the old index consisting of calendarid, uri and creates a new index calendarid, calendartype, uri. And calendartype is the all new column that was introduced by the same migration and set to 0 for all existing entries. So there should be no duplicates.
There was a problem hiding this comment.
@rullzer I was generally not sure what's the cleanest / best approach here.
We have calendars and subscriptions in different tables: calendars respectively calendarsubscriptions. This was no issue so far since we didn't store objects related to subscriptions.
So basically there are two options
- Have duplicate tables of
calendarchanges,calendarobjectsandcalendarobjects_propsfor subscriptions - Introduce
calendartypethat indicates whethercalendaridrefers to thecalendarsor thesubscriptionstable.
There was a problem hiding this comment.
I'm fine with both ways - even if the second one would be cleaner you need to update all the existing queries, right?
There was a problem hiding this comment.
Yes, i guess the big question here is whether introducing the new column calendartype is reducing performance of day-to-day queries.
There was a problem hiding this comment.
When it is used in queries just add an index to it and it should be fine.
|
@tcitworld Can you please test? Are there any other remarks? :) |
|
The acceptance test results are unrelated. The phan thing seems to be a faulty php-doc in Sabre/Dav. |
|
Should I just use |
|
Just wanted to send a pull-request, but it's already fixed upstream: https://github.com/georgehrke/vobject/blob/master/lib/Splitter/ICalendar.php#L86 |
|
@georgehrke is the upstream released and reasonable to get this in? Otherwise, let's suppress it with a note/issue to remove the suppression once we pull again. |
|
The fix was not released yet, adding |
|
@MorrisJobke How to update phan? |
31ac4b6 to
cc66f65
Compare
| /** | ||
| * @param $subscriptionId | ||
| */ | ||
| public function getSubscriptionById($subscriptionId) { |
There was a problem hiding this comment.
When int $subscriptionId works for bigint colums i would use it here.
730c48e to
8967e85
Compare
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
|
Please update the 3rdparty repo due to |
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
8967e85 to
712b79e
Compare
|
Failing tests seem unrelated to me |
rullzer
left a comment
There was a problem hiding this comment.
Code looks good to me. Lets do this.
|
FYI: This caused a regression: #12410 |
|
I accidentally stumpled accross this when debugging an abnormality with my script calcardbackup and noticed that this is not in the changelog for Nextcloud 15 This commit adds events with the same All this are quite big changes which are definitely worth to be mentioned in the changelog. Could this new featur please be added to the changelog? |
fixes #1497
ToDos:
constructoris called beforerun, but we only know the corresponding subscription when we know the arguments forrun