In Koha plugins home page, each plugin min/max version is compared with actual Koha version. This uses metadatas assuming there are numbers, like 18.11 or 20.1100000. But source code version is actually YY.MM.XX.YYY.ZZZ. We sometimes find it in plugins, for examle : https://github.com/thekesolutions/koha-plugin-bibliocommons/blob/c2da98f1bda450452ad687e5623f45939a967bdf/Koha/Plugin/Com/Theke/BiblioCommons.pm#L31 https://github.com/KohaSuomi/koha-plugin-sms-send-link-mobility-driver/blob/master/Koha/Plugin/Fi/KohaSuomi/SMSSendLinkMobilityDriver.pm#L22 This generates warning : Argument "18.11.01" isn't numeric in numeric lt (<) at /home/koha/src/Koha/Plugins/Base.pm line 377.: src/plugins/plugins-home.pl We should allow this syntax in plugins metadatas.
Created attachment 124856 [details] [review] Bug 29008: Avoid warning when checking Koha version in plugins In Koha plugins home page, each plugin min/max version is compared with actual Koha version. This uses metadatas assuming there are numbers, like 18.11 or 20.1100000. But source code version is actually YY.MM.XX.YYY.ZZZ. We sometimes find it in plugins, for examle : https://github.com/thekesolutions/koha-plugin-bibliocommons/blob/c2da98f1bda450452ad687e5623f45939a967bdf/Koha/Plugin/Com/Theke/BiblioCommons.pm#L31 https://github.com/KohaSuomi/koha-plugin-sms-send-link-mobility-driver/blob/master/Koha/Plugin/Fi/KohaSuomi/SMSSendLinkMobilityDriver.pm#L22 This generates warning : Argument "18.11.01" isn't numeric in numeric lt (<) at /home/koha/src/Koha/Plugins/Base.pm line 377.: src/plugins/plugins-home.pl We should allow this syntax in plugins metadatas. Test plan : 1) Install a Koha plugin with minimum_version containing 3 dots : for example 18.11.01.001 2) Go to plugins home page 3) Check there is no warning 4) Install a plugin with minimum_version higher than current Koha version 5) Check in plugins table the warning appears 6) Install a plugin with maximum_version lower than current Koha version 7) Check in plugins table the warning appears
Created attachment 124857 [details] [review] Bug 29008: Add unit test
Created attachment 124858 [details] [review] Bug 29008: Avoid warning when checking Koha version in plugins In Koha plugins home page, each plugin min/max version is compared with actual Koha version. This uses metadatas assuming there are numbers, like 18.11 or 20.1100000. But source code version is actually YY.MM.XX.YYY.ZZZ. We sometimes find it in plugins, for examle : https://github.com/thekesolutions/koha-plugin-bibliocommons/blob/c2da98f1bda450452ad687e5623f45939a967bdf/Koha/Plugin/Com/Theke/BiblioCommons.pm#L31 https://github.com/KohaSuomi/koha-plugin-sms-send-link-mobility-driver/blob/master/Koha/Plugin/Fi/KohaSuomi/SMSSendLinkMobilityDriver.pm#L22 This generates warning : Argument "18.11.01" isn't numeric in numeric lt (<) at /home/koha/src/Koha/Plugins/Base.pm line 377.: src/plugins/plugins-home.pl We should allow this syntax in plugins metadatas. Test plan : 1) Install a Koha plugin with minimum_version containing 3 dots : for example 18.11.01.001 2) Go to plugins home page 3) Check there is no warning 4) Install a plugin with minimum_version higher than current Koha version 5) Check in plugins table the warning appears 6) Install a plugin with maximum_version lower than current Koha version 7) Check in plugins table the warning appears
Created attachment 124859 [details] [review] Bug 29008: Add unit test
I really don't know if it is possible to create unit test on is_for_newer_koha() and is_for_older_koha()
Comparing version numbers is fraught with peril :(.. This error message has annoyed me too on a number of occasions. Do you realise we already have a '_version_compare' routine in Koha::Plugins::Base? Was it a deliberate choice not to use it? Also, In my own plugins, I've started to use Perl version objects for comparisons : https://metacpan.org/dist/version/view/lib/version.pod. I'm wondering if one of these approaches might be more reliable for us?
As an example.. I have ``` use version; sub _version_check { my ( $self, $minversion ) = @_; my $kohaversion = Koha::version(); return ( version->parse($kohaversion) > version->parse($minversion) ); } ``` Which is what I've found to be most reliable to date. In the past I've have ``` sub _version_check { my ( $self, $minversion ) = @_; $minversion =~ s/(.*\..*)\.(.*)\.(.*)/$1$2$3/; my $kohaversion = Koha::version(); # remove the 3 last . to have a Perl number $kohaversion =~ s/(.*\..*)\.(.*)\.(.*)/$1$2$3/; return ( $kohaversion > $minversion ); } ``` The core one is ``` sub _version_compare { my @args = @_; if ( $args[0]->isa('Koha::Plugins::Base') ) { shift @args; } my $ver1 = shift @args || 0; my $ver2 = shift @args || 0; my @v1 = split /[.+:~-]/, $ver1; my @v2 = split /[.+:~-]/, $ver2; for ( my $i = 0 ; $i < max( scalar(@v1), scalar(@v2) ) ; $i++ ) { # Add missing version parts if one string is shorter than the other # i.e. 0 should be lt 0.2.1 and not equal, so we append .0 # 0.0.0 <=> 0.2.1 = -1 push( @v1, 0 ) unless defined( $v1[$i] ); push( @v2, 0 ) unless defined( $v2[$i] ); if ( int( $v1[$i] ) > int( $v2[$i] ) ) { return 1; } elsif ( int( $v1[$i] ) < int( $v2[$i] ) ) { return -1; } } return 0; } ``` And they're all different from what you're doing here...
Ohhh thanks for your alert. I'm sort of lost in those codes. Can you provide a path please ? I'll sign
I've added Kyle and Tomas as CC's here to get their thoughts around the different ways to check versions in Koha plugins.
(In reply to Martin Renvoize from comment #9) > I've added Kyle and Tomas as CC's here to get their thoughts around the > different ways to check versions in Koha plugins. The big problem is we've never defined how plugin versioning works. That being said, I think it's pretty safe to assume semver. In fact, it would be nice to get that set as a community standard. If we can assume semantic versioning, everything because much easier!
(In reply to Martin Renvoize from comment #6) > Comparing version numbers is fraught with peril :(.. > > This error message has annoyed me too on a number of occasions. > > Do you realise we already have a '_version_compare' routine in > Koha::Plugins::Base? Was it a deliberate choice not to use it? The `_version_compare` sub is designed to validate plugin versions, so you can detect a db change is required, there are examples in the wild [1]. Using it actually expects semver but deals with M.m as well. I believe we should use something similar for checking Koha versions, as Frido proposes. But I reckon it would probably be similar to what we do for plugin versions. [1] https://gitlab.com/thekesolutions/plugins/koha-plugin-innreach/-/blob/master/Koha/Plugin/Com/Theke/INNReach.pm#L269-274
(In reply to Kyle M Hall from comment #10) > (In reply to Martin Renvoize from comment #9) > > I've added Kyle and Tomas as CC's here to get their thoughts around the > > different ways to check versions in Koha plugins. > > The big problem is we've never defined how plugin versioning works. That > being said, I think it's pretty safe to assume semver. In fact, it would be > nice to get that set as a community standard. If we can assume semantic > versioning, everything because much easier! We should definitely have something similar to the qa tools, checking this. I already pass my plugin commits through the qa command before pushing :-D Maybe we can add a --plugin param to the command and start checking this annoyances.
(In reply to Tomás Cohen Arazi from comment #12) > (In reply to Kyle M Hall from comment #10) > > (In reply to Martin Renvoize from comment #9) > > > I've added Kyle and Tomas as CC's here to get their thoughts around the > > > different ways to check versions in Koha plugins. > > > > The big problem is we've never defined how plugin versioning works. That > > being said, I think it's pretty safe to assume semver. In fact, it would be > > nice to get that set as a community standard. If we can assume semantic > > versioning, everything because much easier! > > We should definitely have something similar to the qa tools, checking this. > I already pass my plugin commits through the qa command before pushing :-D > Maybe we can add a --plugin param to the command and start checking this > annoyances. Sounds good to me! We could have Koha report invalid semver versions on the plugins page as well.
(In reply to Kyle M Hall from comment #13) > (In reply to Tomás Cohen Arazi from comment #12) > > (In reply to Kyle M Hall from comment #10) > > > (In reply to Martin Renvoize from comment #9) > > > > I've added Kyle and Tomas as CC's here to get their thoughts around the > > > > different ways to check versions in Koha plugins. > > > > > > The big problem is we've never defined how plugin versioning works. That > > > being said, I think it's pretty safe to assume semver. In fact, it would be > > > nice to get that set as a community standard. If we can assume semantic > > > versioning, everything because much easier! > > > > We should definitely have something similar to the qa tools, checking this. > > I already pass my plugin commits through the qa command before pushing :-D > > Maybe we can add a --plugin param to the command and start checking this > > annoyances. > > Sounds good to me! We could have Koha report invalid semver versions on the > plugins page as well. I like your thinking!
This is all very cool thoughts.. but this bug is more around dealing with Koha version comparisons than it is about Plugin versioning ;).
(In reply to Martin Renvoize from comment #15) > This is all very cool thoughts.. but this bug is more around dealing with > Koha version comparisons than it is about Plugin versioning ;). Good point. I think these patches are fine as they are. Again, we could report of the passed in Koha version is non-semver, but in this case it's even less of an issue.
(In reply to Martin Renvoize from comment #15) > This is all very cool thoughts.. but this bug is more around dealing with > Koha version comparisons than it is about Plugin versioning ;). That's why I was confused XD =========== I downloaded the kitchen sink plugin, changed our $MINIMUM_VERSION = "21.05"; ↓↓↓ our $MINIMUM_VERSION = "23.05.01.001"; I see the warning in the UI and Minimum Koha version: 23.05.01.001 But I don't get a warning in the logs. Any ideas?
The patch no longer applies 8-(.... Also, I'm not sure if this needs some more work or if the questions from the discussions are resolved. Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 29008: Avoid warning when checking Koha version in plugins Using index info to reconstruct a base tree... M Koha/Plugins/Base.pm M koha-tmpl/intranet-tmpl/prog/en/modules/plugins/plugins-home.tt M plugins/plugins-home.pl Falling back to patching base and 3-way merge... Auto-merging plugins/plugins-home.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/plugins/plugins-home.tt Auto-merging Koha/Plugins/Base.pm Applying: Bug 29008: Add unit test Using index info to reconstruct a base tree... M t/db_dependent/Koha/Plugins/Plugins.t Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Koha/Plugins/Plugins.t CONFLICT (content): Merge conflict in t/db_dependent/Koha/Plugins/Plugins.t error: Failed to merge in the changes. Patch failed at 0001 Bug 29008: Add unit test
Created attachment 140614 [details] [review] Bug 29008: Add unit test
Created attachment 140615 [details] [review] Bug 29008: Avoid warning when checking Koha version in plugins In Koha plugins home page, each plugin min/max version is compared with actual Koha version. This uses metadatas assuming there are numbers, like 18.11 or 20.1100000. But source code version is actually YY.MM.XX.YYY.ZZZ. We sometimes find it in plugins, for examle : https://github.com/thekesolutions/koha-plugin-bibliocommons/blob/c2da98f1bda450452ad687e5623f45939a967bdf/Koha/Plugin/Com/Theke/BiblioCommons.pm#L31 https://github.com/KohaSuomi/koha-plugin-sms-send-link-mobility-driver/blob/master/Koha/Plugin/Fi/KohaSuomi/SMSSendLinkMobilityDriver.pm#L22 This generates warning : Argument "18.11.01" isn't numeric in numeric lt (<) at /home/koha/src/Koha/Plugins/Base.pm line 377.: src/plugins/plugins-home.pl We should allow this syntax in plugins metadatas. Test plan : 1) Install a Koha plugin with minimum_version containing 3 dots : for example 18.11.01.001 2) Go to plugins home page 3) Check there is no warning 4) Install a plugin with minimum_version higher than current Koha version 5) Check in plugins table the warning appears 6) Install a plugin with maximum_version lower than current Koha version 7) Check in plugins table the warning appears
Created attachment 140616 [details] [review] Bug 29008: Add unit test
Thanks a lot for testing David. I added POD to news methods to avoid a QA warning ;)
I couldn't replicate the warning in the logs before applying the patch. I tried with these plugins: - Downloaded and installed the koha-plugin-bibliocommons plugin (https://github.com/thekesolutions/koha-plugin-bibliocommons/tree/c2da98f1bda450452ad687e5623f45939a967bdf) - Downloaded and installed the dev-koha-plugin-kitchen-sink plugin (https://github.com/bywatersolutions/dev-koha-plugin-kitchen-sink) After changing the version numbers in the plugin .pm file, I: - ran flush_memcached and restart_all - refreshed the plugins page - made the changes as per the test plan Note: I was able to apply the patches, but noticed no difference to the warning messages compared with before the patch was applied. So, I'm a little confused...
So we are two people not getting the warnings: needs more accurate steps to reproduce