Process all biblio records from a Koha instance and generate Sitemap files complying with this protocol as described on http://sitemaps.org. The goal of this script is to be able to provide to search engines direct access to biblio records. It avoid leaving search engine browsing Koha OPAC and so generating a lot of traffic, and workload, for a bad result.
Created attachment 22671 [details] [review] Proposed patch
Created attachment 22679 [details] [review] Proposed patch
1. Typo in the module name, I think Koha::Sitemaper should be Koha::Sitemapper. 2. XML::Writer is not added to C4::Installer::PerlDependencies. 3. The POD for the script talks about generating files called sitemapindex.xml and sitemapXXXX.xml, but as far as I can see, all output is written to STDOUT and has to be split into different files by hand? 4. XML documents output by the script are missing the <?xml ... ?> declarations, but they might be optional? 5. No tests for the stuff in Koha::Sitemaper and Koha::Sitemaper::Writer? Suggestions: 6. It could perhaps be said explicitly that the --url parameter shuld not include the trailing slash? 7. Could the base URL be taken from the OPACBaseURL syspref instead of being a command line parameter? 8. URLs in the sitemap-file are on the form: http://example.org/bib/171 Some sites might have disabled the Apache rewrite rules that make these URLs functional, so it might be better to have the style /cgi-bin/koha/opac-detail.pl?biblionumber=172 as the default and make the shorter form available through a command line switch? I'm marking this "Failed QA" because of number 2 above. But this would make a very cool addition to Koha, so I hope the issues can be fixed! :-)
Thanks Magnus for your feedback. Point 3 is definitely abnormal.
Created attachment 23893 [details] [review] Proposed patch v2 Fixes you points, except 2. Could you retry and describe further what's wrong?
QA preliminary comment : I thought we wanted not to use Moose, and were favoring Class::Accessor
(In reply to Paul Poulain from comment #6) > QA preliminary comment : I thought we wanted not to use Moose, and were > favoring Class::Accessor You're correct, but I think it's rather for CGI scripts which can't support Moose start-up penalty (will be possible with Plack). It's not a problem for a CLI.
Thanks for fixing a lot of issues with this script! I have another couple of suggestons: In regards to issue 3, the problem was on my side: I was executing the script through koha-shell, like so: $ sudo koha-shell -c "perl misc/cronjobs/sitemap.pl -h" kohadev This meant that the script was actually beig run as the kohadev-koha user (or something like that), which did not have write permission to the dir I was executing the script in. This caused the XML to be written to STDOUT, instead of to a file. Would it be hard to add an (optional) switch for specifying where the output goes? If sitemap.pl is run without any arguments, it will create the sitemap files. I seem to remember we have a convention that scripts run without arguments should print a help message and exit, but it does not seem to be in the coding guidelines... Maybe add a --run parameter to actually run the script?
Created attachment 23896 [details] [review] Proposed patch v3 > This meant that the script was actually beig run as the kohadev-koha user > (or something like that), which did not have write permission to the dir I > was executing the script in. This caused the XML to be written to STDOUT, > instead of to a file. I've added a test on this condition: error message is displayed now. > Would it be hard to add an (optional) switch for specifying where the > output goes? Done. > If sitemap.pl is run without any arguments, it will create the sitemap > files. I seem to remember we have a convention that scripts run without > arguments should print a help message and exit, but it does not seem to be > in the coding guidelines... Maybe add a --run parameter to actually run the > script? I'm not sure on this point. For me, a --run is required for scripts which alter database, and when there is a risk of data loose. This isn't the case here.
(In reply to Frédéric Demians from comment #9) > > If sitemap.pl is run without any arguments, it will create the sitemap > > files. I seem to remember we have a convention that scripts run without > > arguments should print a help message and exit, but it does not seem to be > > in the coding guidelines... Maybe add a --run parameter to actually run the > > script? > > I'm not sure on this point. For me, a --run is required for scripts > which alter database, and when there is a risk of data loose. This isn't > the case here. OK. This is really close now, but I have one last problem: It seems that if I run the script without the -dir parameter, no files are created in the current directory, and the script does not complain, with or without -v. But if I run the script with -dir and the absolute path to the current directory as an argument, the sitemap files ar created just fine. I am testing on a gitified package-install, so I have to use sudo to be able to read koha-conf.xml: $ sudo pwd /home/magnus/scripts/kohaclone # No files are creatd in /home/magnus/scripts/kohaclone: $ sudo KOHA_CONF=/etc/koha/sites/kohadev/koha-conf.xml perl misc/cronjobs/sitemap.pl -v Creation of Sitemap files in '.' directory Number of biblio records processed: 172 Number of Sitemap files: 1 # Files *are* created in /home/magnus/scripts/kohaclone: $ sudo KOHA_CONF=/etc/koha/sites/kohadev/koha-conf.xml perl misc/cronjobs/sitemap.pl -v -dir /home/magnus/scripts/kohaclone Creation of Sitemap files in '/home/magnus/scripts/kohaclone' directory Number of biblio records processed: 172 Number of Sitemap files: 1
Created attachment 23979 [details] [review] Proposed patch v4 Thanks Magnus. A tricky bug had been introduced at last minute... Fixed now.
Created attachment 23980 [details] [review] Bug 11190 sitemap.pl -- Generate a Catalog sitemap Add a script sitemap.pl to process all biblio records from a Koha instance and generate Sitemap files complying with this protocol as described on http://sitemaps.org. The goal of this script is to be able to provide to search engines direct access to biblio records. It avoid leaving search engine browsing Koha OPAC and so generating a lot of traffic, and workload, for a bad result. Thanks Magnus for testing, and helping to improve the script design. Signed-off-by: Magnus Enger <magnus@enger.priv.no> All options to the script work as expected and the output looks good. Nice enhancement!
(In reply to Frédéric Demians from comment #7) > (In reply to Paul Poulain from comment #6) > > QA preliminary comment : I thought we wanted not to use Moose, and were > > favoring Class::Accessor > > You're correct, but I think it's rather for CGI scripts which can't support > Moose start-up penalty (will be possible with Plack). It's not a problem for > a CLI. Galen, could you confirm it is not blocker for a push to master?
When I tried to test, the script couldn't find AnyEvent::Processor. Should this library be added as a dependency?
Created attachment 24515 [details] [review] Fix AnyEvent::Processor error AnyEvent::Processor was required, but by error AnyEvent::HTTP was added.
Created attachment 26153 [details] [review] Bug 11190: [SIGNED-OFF] Fix AnyEvent::Processor dependency error Signed-off-by: Magnus Enger <digitalutvikling@gmail.com> Fixes a simple mistake (typo?).
Just to avoid any confusion: I have now signed off both patches on this bug.
Hello, I was studying this patch at the same time as Magnus: I ran the QA scripts and the current patches fail unfortunately. (Looks easy to fix though: documentation and licensing?) Hopes this helps, Alex
Hi there, trying to see what the situation here is - are the QA tools still failing?
... to explain, can't test right now as I am away from my dev environment. Maybe someone can clarify before I can.
(In reply to Katrin Fischer from comment #20) > ... to explain, can't test right now as I am away from my dev environment. > Maybe someone can clarify before I can. I can see QA errors. I will fix them and resubmit the patch with the status "Signed-off'.
Created attachment 34060 [details] [review] Bug 11190 sitemap.pl -- Generate a Catalog sitemap Add a script sitemap.pl to process all biblio records from a Koha instance and generate Sitemap files complying with this protocol as described on http://sitemaps.org. The goal of this script is to be able to provide to search engines direct access to biblio records. It avoid leaving search engine browsing Koha OPAC and so generating a lot of traffic, and workload, for a bad result. Thanks Magnus for testing, and helping to improve the script design. Signed-off-by: Magnus Enger <magnus@enger.priv.no> All options to the script work as expected and the output looks good. Nice enhancement! Signed-off-by: Frederic Demians <f.demians@tamil.fr> I signed-of my own patch after fixing various QA errors.
Created attachment 34061 [details] [review] Bug 11190 sitemap.pl -- Generate a Catalog sitemap Add a script sitemap.pl to process all biblio records from a Koha instance and generate Sitemap files complying with this protocol as described on http://sitemaps.org. The goal of this script is to be able to provide to search engines direct access to biblio records. It avoid leaving search engine browsing Koha OPAC and so generating a lot of traffic, and workload, for a bad result. Thanks Magnus for testing, and helping to improve the script design. Signed-off-by: Magnus Enger <magnus@enger.priv.no> All options to the script work as expected and the output looks good. Nice enhancement! Signed-off-by: Frederic Demians <f.demians@tamil.fr> I signed-of my own patch after fixing various QA errors.
Created attachment 34087 [details] [review] [Passed QA] Bug 11190 sitemap.pl -- Generate a Catalog sitemap Add a script sitemap.pl to process all biblio records from a Koha instance and generate Sitemap files complying with this protocol as described on http://sitemaps.org. The goal of this script is to be able to provide to search engines direct access to biblio records. It avoid leaving search engine browsing Koha OPAC and so generating a lot of traffic, and workload, for a bad result. Thanks Magnus for testing, and helping to improve the script design. Signed-off-by: Magnus Enger <magnus@enger.priv.no> All options to the script work as expected and the output looks good. Nice enhancement! Signed-off-by: Frederic Demians <f.demians@tamil.fr> I signed-of my own patch after fixing various QA errors. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This now passes the QA scripts for me, and appears to be generally ok code for a worthwhile feature. I'll leave the Moose debate to release manager as I'm a little confused on this front.
a small question related to the sitemap size limit. There are 2 limits : 50000 urls + 10MB maximum (unzipped). Has anyone tested with 50000+ url and a "realistic" url site name. I fear that 50k urls would result in more than 10MB file as our urls can be pretty long. For example : http://koha.mediathequeouestprovence.fr/cgi-bin/koha/opac-detail.pl?biblionumber=135438 only for the URL, add headers, timestamps, wouldn't that make a >10MB file ? About the Moose thing: I'm really annoyed. On one hand, this script is very useful and worth being pushed, no doubt. But on the other hand, it add the Moose requirement (although not mandatory), that we removed some months ago. My opinion would be: "OK, let's push this patch, but add a FIXME and a coding guideline to prevent any script like this to be pushed/submitted". (and the "CLI script" argument seems an invalid one to me: otherwise, we could add many depenencies, and that would not be good)
suggestion for a future improvement : be able to add a WHERE clause to the biblio that are sitemap generated. This where could rely on biblio/biblioitems/items parameters. For example, a library could not publish records related to libraryX, or exclude serials, or sitemap only items published before YYYY
Additional (final ?) comment: there is no changefreq definition. I can't find the default value if you don't specify it, but I think that a monthly or even yearly changefreq is better for a search engine (and be careful, for large catalogue, if we put a too frequent period, the server will be crashed by multiple requests. We had to disallow search engines using robots.txt on many libraries !
(In reply to Martin Renvoize from comment #25) > This now passes the QA scripts for me, and appears to be generally ok code > for a worthwhile feature. I'll leave the Moose debate to release manager as > I'm a little confused on this front. When you shut the door, Moose is the kind of thing that enter by the back door... Moo could be used instead. I don't see the advantage, nor the problem with Moose which is such a common dependency in the Perl ecosystem, and which is properly packaged in Debian (and others) now.
(In reply to Paul Poulain from comment #26) > a small question related to the sitemap size limit. > There are 2 limits : 50000 urls + 10MB maximum (unzipped). > Has anyone tested with 50000+ url and a "realistic" url site name. > > I fear that 50k urls would result in more than 10MB file as our urls can be > pretty long. For example : > http://koha.mediathequeouestprovence.fr/cgi-bin/koha/opac-detail. > pl?biblionumber=135438 only for the URL, add headers, timestamps, wouldn't > that make a >10MB file ? I've just tested, for this Koha site URL. A 50000 URL file: - short version = 5.6M - long versoin = 7.3M > (and the "CLI script" argument seems an invalid one to me: otherwise, we > could add many depenencies, and that would not be good) It's not like Moose was a exotic, unmaintained module.
(In reply to Paul Poulain from comment #28) > Additional (final ?) comment: there is no changefreq definition. I can't > find the default value if you don't specify it, but I think that a monthly > or even yearly changefreq is better for a search engine The web crawlers download the sitemap files at an unknown frequency, and then compare the recent URL with the previous one, based on url's timestamp. Even if the sitemap is updated on the Koha server daily, the crawlers won't download it daily. > (and be careful, for > large catalogue, if we put a too frequent period, the server will be crashed > by multiple requests. We had to disallow search engines using robots.txt on > many libraries ! A sitemap helps crawlers not to be lost in a Koha catalog. Does it slow down requests sent by crawlers, we can't say or know. Empirically, I would say yes, but there is no way to be sure. Crawlers seem less tempted to follow multiple links at a rapid pace.
(In reply to Frédéric Demians from comment #31) > (In reply to Paul Poulain from comment #28) > > Additional (final ?) comment: there is no changefreq definition. I can't > > find the default value if you don't specify it, but I think that a monthly > > or even yearly changefreq is better for a search engine > > The web crawlers download the sitemap files at an unknown frequency, and > then compare the recent URL with the previous one, based on url's timestamp. > Even if the sitemap is updated on the Koha server daily, the crawlers won't > download it daily. I think we're not talking of the same thing. http://www.sitemaps.org/protocol.html says =============================== <changefreq> optional How frequently the page is likely to change. This value provides general information to search engines and may not correlate exactly to how often they crawl the page. Valid values are: always hourly daily weekly monthly yearly never The value "always" should be used to describe documents that change each time they are accessed. The value "never" should be used to describe archived URLs. Please note that the value of this tag is considered a hint and not a command. Even though search engine crawlers may consider this information when making decisions, they may crawl pages marked "hourly" less frequently than that, and they may crawl pages marked "yearly" more frequently than that. Crawlers may periodically crawl pages marked "never" so that they can handle unexpected changes to those pages. ================================= It can be defined for each page. My comment was related to each opac-detail.pl page description: we should say to the search engine that the page does not change often, so once it's indexed, there's no need to come back frequently. HTH
(In reply to Paul Poulain from comment #32) > I think we're not talking of the same thing. > http://www.sitemaps.org/protocol.html says > =============================== > <changefreq> optional Yes. I was referring to the script running frequency. The generated sitemap doesn't contain changefreq at all.
I think we agreed after some benchmarking not so use Moose, while Moo seemed to be ok. A problem is, that both are currently not dependencies in Koha and will have to be dealt with for the packaging. As this is an architecture patch, I think it might be worth getting Tomás' opinion and/or adding it to the next dev meeting's agenda.
Created attachment 37942 [details] [review] Bug 11190 sitemap.pl -- Generate a Catalog sitemap Add a script sitemap.pl to process all biblio records from a Koha instance and generate Sitemap files complying with this protocol as described on http://sitemaps.org. The goal of this script is to be able to provide to search engines direct access to biblio records. It avoid leaving search engine browsing Koha OPAC and so generating a lot of traffic, and workload, for a bad result. Thanks Magnus for testing, and helping to improve the script design. [2015.04.16] Switch from Moose to Moo. Signed-off-by: Magnus Enger <magnus@enger.priv.no> All options to the script work as expected and the output looks good. Nice enhancement! Signed-off-by: Frederic Demians <f.demians@tamil.fr> I signed-of my own patch after fixing various QA errors. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Few personal notes about Moose vs Moo (just seen IRC dev meeting logs): There is an overhead using Moose for small short lived programs. But for long- running cronjob/maintenance scripts, there isn't any problem using Moose. For Koha WUI scripts, Moose start time is a problem as long as Koha operates in CGI mode. As soon as there is persistence (Plack), it isn't a problem anymore. This issue of runtime penaly due to using a large and complex library, doing a lot of stuff behind the scene, is not different with Moose and DBIx::Class I don't say DBIx::Class is bad. Moose it bad too => So let's go with Moose! I'd say both libraries share a lot of good things. I think that Moose has various advantages over other OO frameworks. It's complete, very well documented, widely used, a standard, carefully packaged for all major Linux distro. All books about the so-called "modern-Perl movement" deals with using Moose as OO. There is the Perl's famous moto: There's more than one way to do it. Applied to OO, this could conduct to chaos. There are so many ways to do OO in Perl. Code become unreadable and unreliable. Moose forces to adopt a proper and consistent OO approach. Moo try to solve a non-issue (library weight, startup time) in modern execution environments at high price: too many important features are lost.
Created attachment 39712 [details] [review] [SIGNED-OFF] Bug 11190: sitemap.pl -- Generate a Catalog sitemap Add a script sitemap.pl to process all biblio records from a Koha instance and generate Sitemap files complying with this protocol as described on http://sitemaps.org. The goal of this script is to be able to provide to search engines direct access to biblio records. It avoid leaving search engine browsing Koha OPAC and so generating a lot of traffic, and workload, for a bad result. Thanks Magnus for testing, and helping to improve the script design. [2015.04.16] Switch from Moose to Moo. Signed-off-by: Magnus Enger <magnus@enger.priv.no> All options to the script work as expected and the output looks good. Nice enhancement! Signed-off-by: Frederic Demians <f.demians@tamil.fr> I signed-of my own patch after fixing various QA errors. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Fixed conflict on dependencies file, work as expected, no errors.
Frédéric, Could you please provide tests and add the new deps to C4/Installer/PerlDependencies.pm?
(In reply to Jonathan Druart from comment #38) > Frédéric, > Could you please provide tests Do you mean more tests than the minimalist t/Sitemapper.t? > and add the new deps to > C4/Installer/PerlDependencies.pm? Which one other than Moo & XML::Writer?
(In reply to Frédéric Demians from comment #39) > (In reply to Jonathan Druart from comment #38) > > Frédéric, > > Could you please provide tests > > Do you mean more tests than the minimalist t/Sitemapper.t? Yes, if possible. > > and add the new deps to > > C4/Installer/PerlDependencies.pm? > > Which one other than Moo & XML::Writer? Oops, sorry I missed it.
Created attachment 41744 [details] [review] Bug 11190 sitemap.pl -- Generate a Catalog sitemap Add a script sitemap.pl to process all biblio records from a Koha instance and generate Sitemap files complying with this protocol as described on http://sitemaps.org. The goal of this script is to be able to provide to search engines direct access to biblio records. It avoid leaving search engine browsing Koha OPAC and so generating a lot of traffic, and workload, for a bad result. Thanks Magnus for testing, and helping to improve the script design. [2015.04.16] Switch from Moose to Moo. [2015.08.20] Add complere (more) UT. Signed-off-by: Magnus Enger <magnus@enger.priv.no> All options to the script work as expected and the output looks good. Nice enhancement! Signed-off-by: Frederic Demians <f.demians@tamil.fr> I signed-of my own patch after fixing various QA errors. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 41745 [details] [review] Bug 11190 sitemap.pl -- Generate a Catalog sitemap Add a script sitemap.pl to process all biblio records from a Koha instance and generate Sitemap files complying with this protocol as described on http://sitemaps.org. The goal of this script is to be able to provide to search engines direct access to biblio records. It avoid leaving search engine browsing Koha OPAC and so generating a lot of traffic, and workload, for a bad result. Thanks Magnus for testing, and helping to improve the script design. [2015.04.16] Switch from Moose to Moo. [2015.08.20] Add complete (more) UT. Signed-off-by: Magnus Enger <magnus@enger.priv.no> All options to the script work as expected and the output looks good. Nice enhancement! Signed-off-by: Frederic Demians <f.demians@tamil.fr> I signed-of my own patch after fixing various QA errors. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I have set back this patch status to 'Signed off'. The UT should suit QA needs.
(In reply to Frédéric Demians from comment #43) > I have set back this patch status to 'Signed off'. The UT should suit QA > needs. I consider this passed QA, but as Jonathan asked about tests for the features, and the tests have been provided, I think it is Jonathan who should tell.
Created attachment 41989 [details] [review] Bug 11190 sitemap.pl -- Generate a Catalog sitemap Add a script sitemap.pl to process all biblio records from a Koha instance and generate Sitemap files complying with this protocol as described on http://sitemaps.org. The goal of this script is to be able to provide to search engines direct access to biblio records. It avoid leaving search engine browsing Koha OPAC and so generating a lot of traffic, and workload, for a bad result. Thanks Magnus for testing, and helping to improve the script design. [2015.04.16] Switch from Moose to Moo. [2015.08.20] Add complete (more) UT. Signed-off-by: Magnus Enger <magnus@enger.priv.no> All options to the script work as expected and the output looks good. Nice enhancement! Signed-off-by: Frederic Demians <f.demians@tamil.fr> I signed-of my own patch after fixing various QA errors. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Amended patch: replace tabs with spaces.
Feature pushed to master. Awesome work Frederic!
Created attachment 42119 [details] [review] QA followup for tests on jenkins
This patch has been pushed to 3.20.x, will be in 3.20.6.
I am not sure about this one in 3.20 - it's listed as one of the 'new features' for 3.22 and usually new features should not be backported.
I think it's a/a good idea and b/safe to backport this new feature. Why is it safe ? because it's not interlaced with the rest of Koha. So no risk of side-effect. I'm adding that the RMaint is also the sitemap writer, so he'll endorse god-fire in case of death of any kitten anywhere in the world (good luck Fred ;-) )
(In reply to Paul Poulain from comment #50) > I think it's a/a good idea and b/safe to backport this new feature. > Why is it safe ? because it's not interlaced with the rest of Koha. So no > risk of side-effect. > > I'm adding that the RMaint is also the sitemap writer, so he'll endorse > god-fire in case of death of any kitten anywhere in the world > (good luck Fred ;-) ) The only problem are the dependencies, that should be updated for it to work, and will force the users to do a dist-upgrade in order to jump to the minor update. It is uncomfortable, but has been done before, and it is Frederic's call as RMaint.
> The only problem are the dependencies, that should be updated for it to > work, and will force the users to do a dist-upgrade in order to jump to the > minor update. The 2 new dependencies (XML::Writer & Moo) are not mandatory. So their installation isn't necessary until the new script is used.
If I understand Tomas correctly it's still a problem for the packages, even tho the dependencies are non-mandatory.
Changed status to Pushed to Stable
(In reply to Katrin Fischer from comment #53) > If I understand Tomas correctly it's still a problem for the packages, even > tho the dependencies are non-mandatory. In order to use the script: yes, otherwise: no.
I have decided to exclude the patches for this bug from the package for 3.20.6, as I will not build packages if 'make test' fails... which it now does during package construction in 3.20.x. This is because the new dependencies are required for Koha::Sitemap to compile (as is tested by t/00-load.t). I will not add new dependencies because doing so would break apt-get upgrade for the oldstable stream. It was contrary to custom to backport a new feature to a maintenance branch to begin with. While, of course, there is room for some discretion on the part of the RMaint, for the new package maintainer -- i.e. me, adding new dependencies to the package for a maintenance release is a no-go for anything short of a major security issue that has no other plausible solution. The sitemaps feature is not, of course, a security issue. In the case of 3.14.10, which I think was the last time a dist-upgrade was forced for a maintenance release, the patches for bug 8044 at least had the promise of expanding the scope of I18N (although I note that that promise has yet to be fulfilled). I realize that this puts folks in an awkward position. I would prefer that 11190 be reverted from 3.20.x as a matter of policy; alternative approaches to deliver the functionality to users who are not yet ready to upgrade to 3.22.x include establishing a contrib package for the sitemap generator; and if needs must, I can revisit my decision for 3.20.7.
This patch has been reverted from 3.20.x.