It seems that it always expects an array.
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
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>
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>
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...
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>
Good catch, Passing QA
(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.
(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.
Pushed to master for 20.11, thanks to everybody involved!
(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.
backported to 20.05.x for 20.05.06
missing dependencies, not backported to 19.11.x