Created attachment 173980 [details] Holidays module (calendar) I noticed that when I add unique holiday to a library and repeat adding different holidays to two other librarys, the third library will have random unique holidays even if it should't have. It's easier to explain with pictures so I put them in an PDF and added it as an attachment. But here's what I did: 1. I added one week as unique holidays to Tyrnava library from 2025.03.03.to 2025.03.09. All went well. 2. I added one week as unique holidays to Tupos library from 2025.03.10 to 2025.03.16. All went well. 3. I added one week as unique holidays to Tuira library from 2025.03.17 to 2025.03.23. This didin't go so well: Tuira had unique holidays two weeks, but I added only one week. 4. I added one week as unique holidays to Temmes library from 2025.03.03 to 2025.03.09. This didin't go so well either: After saving Temmes had unique holidays three weeks, but I added only one week. 5. I added one week as unique holidays to Taivalkoski library from 2025.03.24 to 2025.03.30. This time after saving Taivalkoski's holidays, library had unique holidays two weeks (in the beginning of March and in the end of March), but I added only one week. These above are easy to fix because you see them at once, but we had also unique holidays that appeared on the other months (for example christmas-holidays appeared to librarys that are open on christmastime and don't need holiday-markings on their calendar.) so those extra-holidays could be hard to catch. We don't have this problem in Koha version 23.11, but in the version 24.05 it is going to be a problem.
Adding my test results to this. I strongly believe this has something to do with plack, since I'm unable to reproduce this on my own test environments where I don't have plack running. We tested this with Piia on our upcoming upgrades test environment and this seems to follow a certain pattern: adding holidays to first and second library works as expected. Then adding holidays to the third one and all the following libraries fail. After restarting plack everything works fine for the first two libraries and then it fails again. Piia also tested this on sandbox and was able reproduce this also there. We had same kind of problem with version 17.05, but it didn't happen again after we upgraded to version 22.11. I'm not sure if this followed same pattern then but symptoms were the same.
Created attachment 174065 [details] [review] Bug 38357: Prevent holidays to be wrongly added This patch resets @holiday_list to an empty array, to prevent holidays to be accumulated in worker's memory. If you added single holidays to library A then others to library B, holidays from A were also added to B. Test plan: Go to /cgi-bin/koha/tools/holidays.pl Define the holidays for Centerville Select a date To: another date (pick only 2 days to ease testing) Set a title "cpl" Tick "Holidays on a range" Save Define the holidays for Fairview Select *another* date To: another date (pick only 2 days to ease testing) Set a title "fpl" Tick "Holidays on a range" Save => Without this patch Fairview has 4 days of holidays instead of 2 => With this patch applied the dates you selected are considered holidays for Fairview
I haven't tried on 23.11 but I didn't find what could have caused this in the git log.
(In reply to Emmi Takkinen from comment #1) > Adding my test results to this. I strongly believe this has something to do > with plack, since I'm unable to reproduce this on my own test environments > where I don't have plack running. We tested this with Piia on our upcoming > upgrades test environment and this seems to follow a certain pattern: adding > holidays to first and second library works as expected. Then adding holidays > to the third one and all the following libraries fail. After restarting > plack everything works fine for the first two libraries and then it fails > again. Piia also tested this on sandbox and was able reproduce this also > there. I get the wrong behaviour for the second library. Can you please try the patch and see if it fixes the problem you described? Maybe there is still something else to fix.
(In reply to Jonathan Druart from comment #4) > (In reply to Emmi Takkinen from comment #1) > > Adding my test results to this. I strongly believe this has something to do > > with plack, since I'm unable to reproduce this on my own test environments > > where I don't have plack running. We tested this with Piia on our upcoming > > upgrades test environment and this seems to follow a certain pattern: adding > > holidays to first and second library works as expected. Then adding holidays > > to the third one and all the following libraries fail. After restarting > > plack everything works fine for the first two libraries and then it fails > > again. Piia also tested this on sandbox and was able reproduce this also > > there. > > I get the wrong behaviour for the second library. Can you please try the > patch and see if it fixes the problem you described? Maybe there is still > something else to fix. Piia tested this on 24.05.01 and it works! She didn't find any other problems so this probably can be signed off. Thank you for quick fix :)
Tested this also in main and calendar works as expected.
Created attachment 174110 [details] [review] Bug 38357: Prevent holidays to be wrongly added This patch resets @holiday_list to an empty array, to prevent holidays to be accumulated in worker's memory. If you added single holidays to library A then others to library B, holidays from A were also added to B. Test plan: Go to /cgi-bin/koha/tools/holidays.pl Define the holidays for Centerville Select a date To: another date (pick only 2 days to ease testing) Set a title "cpl" Tick "Holidays on a range" Save Define the holidays for Fairview Select *another* date To: another date (pick only 2 days to ease testing) Set a title "fpl" Tick "Holidays on a range" Save => Without this patch Fairview has 4 days of holidays instead of 2 => With this patch applied the dates you selected are considered holidays for Fairview Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 174435 [details] [review] Bug 38357: Prevent holidays to be wrongly added This patch resets @holiday_list to an empty array, to prevent holidays to be accumulated in worker's memory. If you added single holidays to library A then others to library B, holidays from A were also added to B. Test plan: Go to /cgi-bin/koha/tools/holidays.pl Define the holidays for Centerville Select a date To: another date (pick only 2 days to ease testing) Set a title "cpl" Tick "Holidays on a range" Save Define the holidays for Fairview Select *another* date To: another date (pick only 2 days to ease testing) Set a title "fpl" Tick "Holidays on a range" Save => Without this patch Fairview has 4 days of holidays instead of 2 => With this patch applied the dates you selected are considered holidays for Fairview Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Pushed for 24.11! Well done everyone, thank you!
Backported to 24.05.x for upcoming 24.05.06
Pushed to 23.11.x for 23.11.11
Not pushed to LTS. Marked Resolved. If you feel this should be in LTS please reply with your reason.
Is there a reason not to get rid of all the `our` variable definitions? I'm not sure we know exactly why it is being used. We only know those variables persist in memory in Plack mode (and other persistent runtime environments).
(In reply to Jesse Maseto from comment #12) > Not pushed to LTS. Marked Resolved. > > If you feel this should be in LTS please reply with your reason. I feel this should be backported if possible.
(In reply to Tomás Cohen Arazi (tcohen) from comment #13) > Is there a reason not to get rid of all the `our` variable definitions? I'm > not sure we know exactly why it is being used. > > We only know those variables persist in memory in Plack mode (and other > persistent runtime environments). It has a global scope, and is available from the subroutines. When switching to plack we (lazily) switched some "my" to "our" to just make things worked. It can be fixed, but it's not simply a s/^our/my/g substitution. But yes, it must be done.
``` my $global; foo(); foo(); warn $global; sub foo { $global++; } ``` => Variable "$global" is not available Replace with `our` => 2
(In reply to Jonathan Druart from comment #15) > (In reply to Tomás Cohen Arazi (tcohen) from comment #13) > > Is there a reason not to get rid of all the `our` variable definitions? I'm > > not sure we know exactly why it is being used. > > > > We only know those variables persist in memory in Plack mode (and other > > persistent runtime environments). > > It has a global scope, and is available from the subroutines. > When switching to plack we (lazily) switched some "my" to "our" to just make > things worked. > > It can be fixed, but it's not simply a s/^our/my/g substitution. > > But yes, it must be done. Yeah, I know. I asked on Mattermost too, if someone remembered some caching mechanism would need to be re-done (i.e. if this was somehow a feature). I think we all agree this needs to go away.
Pushed to 22.11.x. Will be in next point release. 22.11.25