[Bug 11190] New: koha-sitemap.pl -- Generate a Catalog sitemap
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Bug ID: 11190 Summary: koha-sitemap.pl -- Generate a Catalog sitemap Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: new feature Priority: P5 - low Component: Command-line Utilities Assignee: gmcharlt@gmail.com Reporter: frederic@tamil.fr QA Contact: testopia@bugs.koha-community.org 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |magnus@enger.priv.no Depends on| |6594 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|koha-sitemap.pl -- Generate |sitemap.pl -- Generate a |a Catalog sitemap |Catalog sitemap -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Assignee|gmcharlt@gmail.com |frederic@tamil.fr --- Comment #1 from Frédéric Demians <frederic@tamil.fr> --- Created attachment 22671 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22671&action=edit Proposed patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #22671|0 |1 is obsolete| | --- Comment #2 from Frédéric Demians <frederic@tamil.fr> --- Created attachment 22679 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22679&action=edit Proposed patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Magnus Enger <magnus@enger.priv.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #3 from Magnus Enger <magnus@enger.priv.no> --- 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! :-) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #4 from Frédéric Demians <frederic@tamil.fr> --- Thanks Magnus for your feedback. Point 3 is definitely abnormal. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #22679|0 |1 is obsolete| | --- Comment #5 from Frédéric Demians <frederic@tamil.fr> --- Created attachment 23893 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23893&action=edit Proposed patch v2 Fixes you points, except 2. Could you retry and describe further what's wrong? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |paul.poulain@biblibre.com --- Comment #6 from Paul Poulain <paul.poulain@biblibre.com> --- QA preliminary comment : I thought we wanted not to use Moose, and were favoring Class::Accessor -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #7 from Frédéric Demians <frederic@tamil.fr> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #8 from Magnus Enger <magnus@enger.priv.no> --- 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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #23893|0 |1 is obsolete| | --- Comment #9 from Frédéric Demians <frederic@tamil.fr> --- Created attachment 23896 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23896&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Magnus Enger <magnus@enger.priv.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #10 from Magnus Enger <magnus@enger.priv.no> --- (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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #23896|0 |1 is obsolete| | --- Comment #11 from Frédéric Demians <frederic@tamil.fr> --- Created attachment 23979 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23979&action=edit Proposed patch v4 Thanks Magnus. A tricky bug had been introduced at last minute... Fixed now. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Magnus Enger <magnus@enger.priv.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #12 from Magnus Enger <magnus@enger.priv.no> --- Created attachment 23980 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23980&action=edit 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! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Magnus Enger <magnus@enger.priv.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #23979|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |gmcharlt@gmail.com, | |jonathan.druart@biblibre.co | |m --- Comment #13 from Jonathan Druart <jonathan.druart@biblibre.com> --- (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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |kyle@bywatersolutions.com --- Comment #14 from Kyle M Hall <kyle@bywatersolutions.com> --- When I tried to test, the script couldn't find AnyEvent::Processor. Should this library be added as a dependency? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #15 from Frédéric Demians <frederic@tamil.fr> --- Created attachment 24515 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=24515&action=edit Fix AnyEvent::Processor error AnyEvent::Processor was required, but by error AnyEvent::HTTP was added. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Magnus Enger <magnus@enger.priv.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Magnus Enger <magnus@enger.priv.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #24515|0 |1 is obsolete| | --- Comment #16 from Magnus Enger <magnus@enger.priv.no> --- Created attachment 26153 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=26153&action=edit Bug 11190: [SIGNED-OFF] Fix AnyEvent::Processor dependency error Signed-off-by: Magnus Enger <digitalutvikling@gmail.com> Fixes a simple mistake (typo?). -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #17 from Magnus Enger <magnus@enger.priv.no> --- Just to avoid any confusion: I have now signed off both patches on this bug. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Alex Sassmannshausen <alex.sassmannshausen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |alex.sassmannshausen@gmail. | |com --- Comment #18 from Alex Sassmannshausen <alex.sassmannshausen@gmail.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #19 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Hi there, trying to see what the situation here is - are the QA tools still failing? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #20 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- ... to explain, can't test right now as I am away from my dev environment. Maybe someone can clarify before I can. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #21 from Frédéric Demians <frederic@tamil.fr> --- (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'. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #22 from Frédéric Demians <frederic@tamil.fr> --- Created attachment 34060 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=34060&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #23980|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #26153|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #34060|0 |1 is obsolete| | --- Comment #23 from Frédéric Demians <frederic@tamil.fr> --- Created attachment 34061 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=34061&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #34061|0 |1 is obsolete| | --- Comment #24 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 34087 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=34087&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com --- Comment #25 from 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #26 from Paul Poulain <paul.poulain@biblibre.com> --- 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?biblionu... 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) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #27 from Paul Poulain <paul.poulain@biblibre.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #28 from Paul Poulain <paul.poulain@biblibre.com> --- 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 ! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #29 from Frédéric Demians <frederic@tamil.fr> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #30 from Frédéric Demians <frederic@tamil.fr> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #31 from Frédéric Demians <frederic@tamil.fr> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #32 from Paul Poulain <paul.poulain@biblibre.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #33 from Frédéric Demians <frederic@tamil.fr> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #34 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #34087|0 |1 is obsolete| | --- Comment #35 from Frédéric Demians <frederic@tamil.fr> --- Created attachment 37942 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=37942&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #36 from Frédéric Demians <frederic@tamil.fr> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #37942|0 |1 is obsolete| | --- Comment #37 from Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> --- Created attachment 39712 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=39712&action=edit [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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #38 from Jonathan Druart <jonathan.druart@biblibre.com> --- Frédéric, Could you please provide tests and add the new deps to C4/Installer/PerlDependencies.pm? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #39 from Frédéric Demians <frederic@tamil.fr> --- (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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #40 from Jonathan Druart <jonathan.druart@biblibre.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #41 from Frédéric Demians <frederic@tamil.fr> --- Created attachment 41744 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41744&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #42 from Frédéric Demians <frederic@tamil.fr> --- Created attachment 41745 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41745&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #39712|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #41744|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #43 from Frédéric Demians <frederic@tamil.fr> --- I have set back this patch status to 'Signed off'. The UT should suit QA needs. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Kyle M Hall <kyle.m.hall@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC|kyle@bywatersolutions.com |kyle.m.hall@gmail.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |jonathan.druart@bugs.koha-c |y.org |ommunity.org -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #44 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #41745|0 |1 is obsolete| | --- Comment #45 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 41989 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41989&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #46 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Feature pushed to master. Awesome work Frederic! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #47 from Frédéric Demians <frederic@tamil.fr> --- Created attachment 42119 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42119&action=edit QA followup for tests on jenkins -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|sitemap.pl -- Generate a |sitemap.pl -- Generate a |Catalog sitemap |catalog sitemap -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #48 from Frédéric Demians <frederic@tamil.fr> --- This patch has been pushed to 3.20.x, will be in 3.20.6. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |julian.maurice@biblibre.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #49 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #50 from Paul Poulain <paul.poulain@biblibre.com> --- 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 ;-) ) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #51 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #52 from Frédéric Demians <frederic@tamil.fr> ---
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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |dependency CC| |gmc@esilibrary.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #53 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- If I understand Tomas correctly it's still a problem for the packages, even tho the dependencies are non-mandatory. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable --- Comment #54 from Julian Maurice <julian.maurice@biblibre.com> --- Changed status to Pushed to Stable -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #55 from Frédéric Demians <frederic@tamil.fr> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #56 from Galen Charlton <gmcharlt@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11190 --- Comment #57 from Frédéric Demians <frederic@tamil.fr> --- This patch has been reverted from 3.20.x. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org