[RESEND] Ban the use of `our` in our codebase?
I'm not sure why we didn't do it already, but running Koha in a persistent context has become the standard. This means variables defined as `our $variable` will persist in memory while workers run. And this is dangerous for several reasons. The most obvious one is that sometimes the code expects the caller to pass a parameter and checks if it is defined, and using `our` might make it defined when it shouldn't. If the reason to use it is caching, then it is a bad pattern anyway. We should be using the in-memory cache (lite) explicitly so everyone knows the value is expected to be fetched from a cache. Looking forward to hear your thoughts PS interestingly, I found this while finding the root cause for some files not being tidied :-P https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39152 -- Tomás Cohen Arazi Theke Solutions (https://theke.io) ✆ +54 9351 3513384 GPG: B2F3C15F
My answer from bug 38357 comment 15: """ 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 work. It can be fixed, but it's not simply a s/^our/my/g substitution. But yes, it must be done. """ Le mer. 12 mars 2025 à 11:56, Tomas Cohen Arazi via Koha-devel <koha-devel@lists.koha-community.org> a écrit :
I'm not sure why we didn't do it already, but running Koha in a persistent context has become the standard.
This means variables defined as `our $variable` will persist in memory while workers run. And this is dangerous for several reasons. The most obvious one is that sometimes the code expects the caller to pass a parameter and checks if it is defined, and using `our` might make it defined when it shouldn't.
If the reason to use it is caching, then it is a bad pattern anyway. We should be using the in-memory cache (lite) explicitly so everyone knows the value is expected to be fetched from a cache.
Looking forward to hear your thoughts
PS interestingly, I found this while finding the root cause for some files not being tidied :-P https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39152
-- Tomás Cohen Arazi Theke Solutions (https://theke.io) ✆ +54 9351 3513384 GPG: B2F3C15F _______________________________________________ Koha-devel mailing list Koha-devel@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel website : https://www.koha-community.org/ git : https://git.koha-community.org/ bugs : https://bugs.koha-community.org/
I believe you are correct. IIRC some of the times we've attempted to simply swap `our` for `my` it has created issues with the code. Swapping the variable for a caching mechanism definitely sounds like the way to go! I'm seeing a lot of `our` keywords that appear to be functionally constants, and should be changed to constants. Kyle --- http://www.kylehall.info ByWater Solutions ( http://bywatersolutions.com ) Meadville Public Library ( http://www.meadvillelibrary.org ) Crawford County Federated Library System ( http://www.ccfls.org ) On Wed, Mar 12, 2025 at 6:56 AM Tomas Cohen Arazi via Koha-devel < koha-devel@lists.koha-community.org> wrote:
I'm not sure why we didn't do it already, but running Koha in a persistent context has become the standard.
This means variables defined as `our $variable` will persist in memory while workers run. And this is dangerous for several reasons. The most obvious one is that sometimes the code expects the caller to pass a parameter and checks if it is defined, and using `our` might make it defined when it shouldn't.
If the reason to use it is caching, then it is a bad pattern anyway. We should be using the in-memory cache (lite) explicitly so everyone knows the value is expected to be fetched from a cache.
Looking forward to hear your thoughts
PS interestingly, I found this while finding the root cause for some files not being tidied :-P https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39152
-- Tomás Cohen Arazi Theke Solutions (https://theke.io) ✆ +54 9351 3513384 GPG: B2F3C15F _______________________________________________ Koha-devel mailing list Koha-devel@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel website : https://www.koha-community.org/ git : https://git.koha-community.org/ bugs : https://bugs.koha-community.org/
Yeah, I'd advocate for a 'fix as seen' basis instead of a whole rewrite project. But yeah. El mié, 12 mar 2025 a las 8:18, Kyle Hall (<kyle.m.hall@gmail.com>) escribió:
I believe you are correct. IIRC some of the times we've attempted to simply swap `our` for `my` it has created issues with the code. Swapping the variable for a caching mechanism definitely sounds like the way to go! I'm seeing a lot of `our` keywords that appear to be functionally constants, and should be changed to constants.
Kyle
--- http://www.kylehall.info ByWater Solutions ( http://bywatersolutions.com ) Meadville Public Library ( http://www.meadvillelibrary.org ) Crawford County Federated Library System ( http://www.ccfls.org )
On Wed, Mar 12, 2025 at 6:56 AM Tomas Cohen Arazi via Koha-devel < koha-devel@lists.koha-community.org> wrote:
I'm not sure why we didn't do it already, but running Koha in a persistent context has become the standard.
This means variables defined as `our $variable` will persist in memory while workers run. And this is dangerous for several reasons. The most obvious one is that sometimes the code expects the caller to pass a parameter and checks if it is defined, and using `our` might make it defined when it shouldn't.
If the reason to use it is caching, then it is a bad pattern anyway. We should be using the in-memory cache (lite) explicitly so everyone knows the value is expected to be fetched from a cache.
Looking forward to hear your thoughts
PS interestingly, I found this while finding the root cause for some files not being tidied :-P https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39152
-- Tomás Cohen Arazi Theke Solutions (https://theke.io) ✆ +54 9351 3513384 GPG: B2F3C15F _______________________________________________ Koha-devel mailing list Koha-devel@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel website : https://www.koha-community.org/ git : https://git.koha-community.org/ bugs : https://bugs.koha-community.org/
-- Tomás Cohen Arazi Theke Solutions (https://theke.io) ✆ +54 9351 3513384 GPG: B2F3C15F
participants (3)
-
Jonathan Druart -
Kyle Hall -
Tomas Cohen Arazi