Let's assume you have an eventseries, with 3 custom dates:
- 1st of april, 2024, 10:00 - 12:00
- 2nd of april 2024, 10:00 - 12:00
- 3rd of april 2024, 10:00 - 12:00
Now, let's say that we want to add a 4th of april.
If an editor goes to the series edit page, they and add the 4th of april, the 3 existing instances will be deleted, and re-created.
This is a problem for anything that uses APIs, as the instances will get new IDs.
It is also a problem if any of the existing instances have had custom values added.
I can see in the code that we have a "events_to_create" array. I'd expect this array to loop through, and check that if any date combination already exists, we should not delete that - and should not create it again.
The same concept also applies for non-custom - if you update an series that only happened on tuesdays, to also happen on wednesdays.
I have a merge request ready, that I will attach. It also cleans up some of the related code, as it was duplicated.
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
abrammThe MR doesn't correctly account included and excluded dates; that's likely because include/exclude logic is handled in the
recurring_events_event_instances_pre_createhook implementation (recurring_events_recurring_events_event_instances_pre_create_alter). This causes an issue with the removal of instances that are no longer in the range.The proposed code change calls this hook only for events instances to be created while originally it was called for *all* instances.
Steps to reproduce:
1) Create the weekly event series, starting from e.g. the beginning of the year and ending at the end of the year.
2) Observe 51 instances (or whatever the number of weeks is in range) created.
3) Change the included date range to include only one month from the range, save the series.
Expected: all instances except those in the included range are deleted.
Actual: none of the instances are deleted.
Note: I'm talking about the Excluded dates and Included dates fields, which are below the normal range selection.
Comment #4
abrammComment #5
abrammI've pushed a fix for the issue outlined in #3 (hope @ras-ben doesn't mind; otherwise, feel free to remove/squash my commit),
Attaching a patch to apply via composer.json.
Comment #6
kasperg commented> The MR doesn't correctly account included and excluded dates.
I tested the updated MR by creating a weekly event over a year and excluding 11 of the 12 months. This resulted in removal of the expected events while the others were left untouched.
Comment #7
pfrenssenBumping this to major since it can cause unexpected data loss.
Let's add some test coverage for this so we can ascertain that this works as expected.
Comment #8
pfrenssenI had a quick look at the patch but in its current form it is not yet taking advantage of the
EventInstanceCreatorplugin system that was added in #3298679: By updating a series, it deletes all the eventinstances and recreates them, which deletes all information stored on the instance. with the specific intent to make this use case possible in a flexible way.We should definitely make use of this, and add a new plugin like an
UnchangedDateRangePreservingEventInstanceCreatorthat users can opt-in to.Assigning to do an in-depth review.
Comment #9
pfrenssenComment #10
ras-ben commentedI think your comments are excellent @pfrenssen, but I dont know if I'll have the time this month to look into fixing them - so, if anyone has the time and expertise, you are very welcome :)
Comment #11
awolfey commentedShould this bug be critical? It's on the edge, but it does cause loss of existing data in custom fields added to the instance entity.
Comment #12
pfrenssen@awolfey, no this is not critical. It is also not a bug, this is just how it was originally designed. There is a very clear warning shown to the user that the data loss will occur and they have to confirm it.
This is a known shortcoming. The groundwork for solving this has been laid in #3298679: By updating a series, it deletes all the eventinstances and recreates them, which deletes all information stored on the instance.. There is also very interesting prior discussion in that issue.
There is also #3479449: Provide a 'NoOperationEventInstanceCreator' that prevents event instances from changing when event series is saved which has a simple but very effective solution that can be implemented as a hook and is waiting for review.