Bug 26751

Summary: Fatal exception if only one repo defined
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Plugin architectureAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: aleisha, dcook, lucas, martin.renvoize
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.06
Bug Depends on: 23975    
Bug Blocks: 24633    
Attachments: Bug 26751: Fix fatal exception if only one repo defined
Bug 26751: Fix fatal exception if only one repo defined
Bug 26751: Fix fatal exception if only one repo defined

Description Tomás Cohen Arazi 2020-10-20 12:41:07 UTC
It seems that it always expects an array.
Comment 1 Tomás Cohen Arazi 2020-10-20 12:42:25 UTC
Steps to reproduce:
1. Have several entries for querying
2. Search for plugins
=> SUCCESS: It doesn't die
3. Have only one entry
4. Repeat 2
=> FAIL: It dies
Comment 2 Tomás Cohen Arazi 2020-10-20 17:45:17 UTC
Created attachment 112065 [details] [review]
Bug 26751: Fix fatal exception if only one repo defined

This patch fixes this nasty problem with how XML is parsed: if more than
one entry is found, repo => [] is generated. If only one, repo => {}.

To test:
1. Have more than one entries in the plugins_repo entry:

<plugin_repos>
    <repo>
        <name>ByWater Solutions</name>
        <org_name>bywatersolutions</org_name>
        <service>github</service>
    </repo>
    <repo>
        <name>Theke Solutions</name>
        <org_name>thekesolutions</org_name>
        <service>github</service>
    </repo>
    <repo>
        <name>PTFS Europe</name>
        <org_name>ptfs-europe</org_name>
        <service>github</service>
    </repo>
 </plugin_repos>

2. Search for plugins
=> SUCCESS: Searching works!
3. Leave only one entry
4. Restart all to refresh things
5. Try searching for plugins
=> FAIL: Fatal error
6. Apply this patch
7. Repeat 5
=> SUCCESS: It doesn't explode anymore!
8. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Kyle M Hall 2020-10-20 18:13:05 UTC
Created attachment 112070 [details] [review]
Bug 26751: Fix fatal exception if only one repo defined

This patch fixes this nasty problem with how XML is parsed: if more than
one entry is found, repo => [] is generated. If only one, repo => {}.

To test:
1. Have more than one entries in the plugins_repo entry:

<plugin_repos>
    <repo>
        <name>ByWater Solutions</name>
        <org_name>bywatersolutions</org_name>
        <service>github</service>
    </repo>
    <repo>
        <name>Theke Solutions</name>
        <org_name>thekesolutions</org_name>
        <service>github</service>
    </repo>
    <repo>
        <name>PTFS Europe</name>
        <org_name>ptfs-europe</org_name>
        <service>github</service>
    </repo>
 </plugin_repos>

2. Search for plugins
=> SUCCESS: Searching works!
3. Leave only one entry
4. Restart all to refresh things
5. Try searching for plugins
=> FAIL: Fatal error
6. Apply this patch
7. Repeat 5
=> SUCCESS: It doesn't explode anymore!
8. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 4 David Cook 2020-10-21 00:20:53 UTC
One of the unfortunate side-effects of using XML::Simple :(

We really do need to get rid of it one day, although I wonder how many issues will be created as a result...
Comment 5 Martin Renvoize 2020-10-21 12:35:40 UTC
Created attachment 112096 [details] [review]
Bug 26751: Fix fatal exception if only one repo defined

This patch fixes this nasty problem with how XML is parsed: if more than
one entry is found, repo => [] is generated. If only one, repo => {}.

To test:
1. Have more than one entries in the plugins_repo entry:

<plugin_repos>
    <repo>
        <name>ByWater Solutions</name>
        <org_name>bywatersolutions</org_name>
        <service>github</service>
    </repo>
    <repo>
        <name>Theke Solutions</name>
        <org_name>thekesolutions</org_name>
        <service>github</service>
    </repo>
    <repo>
        <name>PTFS Europe</name>
        <org_name>ptfs-europe</org_name>
        <service>github</service>
    </repo>
 </plugin_repos>

2. Search for plugins
=> SUCCESS: Searching works!
3. Leave only one entry
4. Restart all to refresh things
5. Try searching for plugins
=> FAIL: Fatal error
6. Apply this patch
7. Repeat 5
=> SUCCESS: It doesn't explode anymore!
8. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2020-10-21 12:36:40 UTC
Good catch, Passing QA
Comment 7 David Cook 2020-10-21 23:15:52 UTC
(In reply to Martin Renvoize from comment #5)
> This patch fixes this nasty problem with how XML is parsed: if more than
> one entry is found, repo => [] is generated. If only one, repo => {}.

To nitpick, the nasty problem is just due to XML::Simple being problematic. Grant McLean (who is at Kohacon2020 and works at Catalyst IT) doesn't that people use it in new code, and I reckon we should work on just taking it out of our code.

By the way, an alternative to this patch would be to update the forcearray option in Koha::Config::read_from_file. (See https://metacpan.org/pod/XML::Simple#ForceArray-=%3E-1-%23-in-important for more information)

In fact, it might be better to use my alternative idea, because then we'd have more centralized in-code documentation about what config options *need* to be arrays.
Comment 8 Tomás Cohen Arazi 2020-10-22 01:36:01 UTC
(In reply to David Cook from comment #7)
> (In reply to Martin Renvoize from comment #5)
> > This patch fixes this nasty problem with how XML is parsed: if more than
> > one entry is found, repo => [] is generated. If only one, repo => {}.
> 
> To nitpick, the nasty problem is just due to XML::Simple being problematic.
> Grant McLean (who is at Kohacon2020 and works at Catalyst IT) doesn't that
> people use it in new code, and I reckon we should work on just taking it out
> of our code.
> 
> By the way, an alternative to this patch would be to update the forcearray
> option in Koha::Config::read_from_file. (See
> https://metacpan.org/pod/XML::Simple#ForceArray-=%3E-1-%23-in-important for
> more information)
> 
> In fact, it might be better to use my alternative idea, because then we'd
> have more centralized in-code documentation about what config options *need*
> to be arrays.

I'd say we should leave this bug alone, and propose the refactoring in its own bug, so stable branches can have this feature fixed. Also there might be other options to explore. I proposed to move things into YAML or JSON, splitting the Zebra and general config for good.
Comment 9 Jonathan Druart 2020-10-22 08:25:41 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 10 David Cook 2020-10-22 22:30:57 UTC
(In reply to Tomás Cohen Arazi from comment #8)
> I'd say we should leave this bug alone, and propose the refactoring in its
> own bug, so stable branches can have this feature fixed. Also there might be
> other options to explore. I proposed to move things into YAML or JSON,
> splitting the Zebra and general config for good.

I think using the XML::Simple "forcearray" option may have been a good idea in this case, but this patch works too.

But I agree about a new bug about refactoring configuration. I've opened bug 26790.
Comment 11 Lucas Gass 2020-11-13 18:19:54 UTC
backported to 20.05.x for 20.05.06
Comment 12 Aleisha Amohia 2020-11-17 04:15:45 UTC
missing dependencies, not backported to 19.11.x