Bug 29008 - Warning when checking Koha version in plugins
Summary: Warning when checking Koha version in plugins
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Fridolin Somers
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 25790
  Show dependency treegraph
 
Reported: 2021-09-14 06:25 UTC by Fridolin Somers
Modified: 2022-10-18 03:14 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 29008: Avoid warning when checking Koha version in plugins (4.49 KB, patch)
2021-09-14 09:19 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 29008: Add unit test (1.59 KB, patch)
2021-09-14 09:22 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 29008: Avoid warning when checking Koha version in plugins (4.51 KB, patch)
2021-09-14 09:22 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 29008: Add unit test (1.59 KB, patch)
2021-09-14 09:23 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 29008: Add unit test (1.59 KB, patch)
2022-09-14 09:17 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 29008: Avoid warning when checking Koha version in plugins (4.86 KB, patch)
2022-09-14 09:17 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 29008: Add unit test (1.59 KB, patch)
2022-09-14 09:17 UTC, Fridolin Somers
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2021-09-14 06:25:39 UTC
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.
Comment 1 Fridolin Somers 2021-09-14 09:19:55 UTC
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
Comment 2 Fridolin Somers 2021-09-14 09:22:18 UTC
Created attachment 124857 [details] [review]
Bug 29008: Add unit test
Comment 3 Fridolin Somers 2021-09-14 09:22:34 UTC
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
Comment 4 Fridolin Somers 2021-09-14 09:23:03 UTC
Created attachment 124859 [details] [review]
Bug 29008: Add unit test
Comment 5 Fridolin Somers 2021-09-14 09:57:05 UTC
I really don't know if it is possible to create unit test on is_for_newer_koha() and is_for_older_koha()
Comment 6 Martin Renvoize 2021-11-02 15:37:48 UTC
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?
Comment 7 Martin Renvoize 2021-11-02 15:41:50 UTC
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...
Comment 8 Fridolin Somers 2021-11-03 06:53:14 UTC
Ohhh thanks for your alert.

I'm sort of lost in those codes.
Can you provide a path please ? I'll sign
Comment 9 Martin Renvoize 2022-04-11 11:03:43 UTC
I've added Kyle and Tomas as CC's here to get their thoughts around the different ways to check versions in Koha plugins.
Comment 10 Kyle M Hall 2022-04-11 12:07:03 UTC
(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!
Comment 11 Tomás Cohen Arazi 2022-04-11 13:04:22 UTC
(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
Comment 12 Tomás Cohen Arazi 2022-04-11 13:10:47 UTC
(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.
Comment 13 Kyle M Hall 2022-04-11 14:42:36 UTC
(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.
Comment 14 Tomás Cohen Arazi 2022-04-11 14:56:30 UTC
(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!
Comment 15 Martin Renvoize 2022-04-11 16:49:47 UTC
This is all very cool thoughts.. but this bug is more around dealing with Koha version comparisons than it is about Plugin versioning ;).
Comment 16 Kyle M Hall 2022-04-11 17:07:27 UTC
(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.
Comment 17 Victor Grousset/tuxayo 2022-06-30 16:58:07 UTC
(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?
Comment 18 David Nind 2022-09-13 19:29:38 UTC
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
Comment 19 Fridolin Somers 2022-09-14 09:17:10 UTC
Created attachment 140614 [details] [review]
Bug 29008: Add unit test
Comment 20 Fridolin Somers 2022-09-14 09:17:51 UTC
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
Comment 21 Fridolin Somers 2022-09-14 09:17:55 UTC
Created attachment 140616 [details] [review]
Bug 29008: Add unit test
Comment 22 Fridolin Somers 2022-09-14 09:18:59 UTC
Thanks a lot for testing David.

I added POD to news methods to avoid a QA warning ;)
Comment 23 David Nind 2022-10-10 21:54:10 UTC
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...
Comment 24 Victor Grousset/tuxayo 2022-10-18 03:14:04 UTC
So we are two people not getting the warnings: needs more accurate steps to reproduce