As a result of (ancient) commit f6e1214c, OPAC search result RSS feeds come over with the content type text/html. It should be at least text/xml, and with the demise of Internet Explorer 6, arguably application/rss+xml. The cause: prior to that commit, setting the CGI parameter format to rss2 resulted in passing "rss" as the content type parameter to output_with_http_headers; after that commit, rss2 was no longer properly recognized when setting that parameter. The main user-visible consequence is that clicking on the RSS link in Chrome (when no feed-reader plugin is installed) renders the feed as HTML rather than XML.
Created attachment 49400 [details] [review] Bug 16111 - RSS feed for OPAC search results has wrong content type This patch fixing handling of OPAC search results feeds by correcting parameters passed to the script and making format checks consistent in the script. This patch also removes altogether references to a separate "atom" formatted feed, because the template for that feed displays no content (and never has). To test, apply the patch and perform a search in the OPAC. - Click the RSS link and confirm that the browser displays a feed with no errors. In Chrome the feed should display a "document tree" view. - Using the browser's developer console, verify that the content-type of the document is defined as "text/xml." - Validate the resulting feed at https://validator.w3.org/feed/ and confirm that it is valid.
Comment on attachment 49400 [details] [review] Bug 16111 - RSS feed for OPAC search results has wrong content type Review of attachment 49400 [details] [review]: ----------------------------------------------------------------- ::: opac/opac-search.pl @@ +733,4 @@ > } > > ## If there's just one result, redirect to the detail page > + if ($total == 1 && $format ne 'rss' && $format ne 'opensearchdescription' ) { I've no objection to switching to 'rss', but 'rss2' ought to continue to be recognized as a format specifier; otherwise, we'll break existing feed subscriptions. Quick addition to do this would be along the lines of my $format = $cgi->param("format") || ''; $format = 'rss' if $format =~ /rss/;
Created attachment 49587 [details] [review] Bug 16111 [Revised] RSS feed for OPAC search results has wrong content type This patch fixing handling of OPAC search results feeds by correcting parameters passed to the script and making format checks consistent in the script. This patch also removes altogether references to a separate "atom" formatted feed, because the template for that feed displays no content (and never has). To test, apply the patch and perform a search in the OPAC. - Click the RSS link and confirm that the browser displays a feed with no errors. In Chrome the feed should display a "document tree" view. - Using the browser's developer console, verify that the content-type of the document is defined as "text/xml." - Validate the resulting feed at https://validator.w3.org/feed/ and confirm that it is valid. Revision adds suggestion from QA to accept the old 'rss2' parameter and treat it like an 'rss' parameter. Also changed: Corrected another place where 'rss2' was used in the template.
Testing in Firefox 45.0. Without the patch: - Do a search - Click on the RSS icon - Verify the response has a text/html content type With the patch: - Do the same search - Click on the RSS icon - The response is now text/xml. Good! - But: Boom, Firefox shows this error: XML Parsing Error: no element found Location: http://kohadev/cgi-bin/koha/opac-search.pl?idx=kw&q=perl&count=50&sort_by=acqdate_dsc&format=rss2 Line Number 10, Column 1: And the response from the server is just 10 blank lines, no content at all. For added weirdness: Chromium displays the RSS seemingly OK after applying the patch.
Please disregard comment #4, I was not testing with English as the chosen language. Guess I am a bit rusty...
Created attachment 49629 [details] [review] Bug 16111 [Revised] RSS feed for OPAC search results has wrong content type This patch fixing handling of OPAC search results feeds by correcting parameters passed to the script and making format checks consistent in the script. This patch also removes altogether references to a separate "atom" formatted feed, because the template for that feed displays no content (and never has). To test, apply the patch and perform a search in the OPAC. - Click the RSS link and confirm that the browser displays a feed with no errors. In Chrome the feed should display a "document tree" view. - Using the browser's developer console, verify that the content-type of the document is defined as "text/xml." - Validate the resulting feed at https://validator.w3.org/feed/ and confirm that it is valid. Revision adds suggestion from QA to accept the old 'rss2' parameter and treat it like an 'rss' parameter. Also changed: Corrected another place where 'rss2' was used in the template. Signed-off-by: Magnus Enger <magnus@librietech.no> Before the patch: - RSS is served with content-type text/html - The link from the RSS icon ona search page looks like opac-search.pl?idx=kw&q=perl&count=50&sort_by=acqdate_dsc&format=rss2 After applying the patch: - RSS is served as text/xml - The RSS link leads to opac-search.pl?idx=kw&q=perl&count=50&sort_by=acqdate_dsc&format=rss But the old link with format=rss2 still works
Would not it better to fix the atom feeds instead of removing it?
(In reply to Jonathan Druart from comment #7) > Would not it better to fix the atom feeds instead of removing it? Making the atom feed work would really be a new feature, since as far as I can tell they were never actually implemented.
Created attachment 49837 [details] [review] Bug 16111: POC
(In reply to Owen Leonard from comment #8) > (In reply to Jonathan Druart from comment #7) > > Would not it better to fix the atom feeds instead of removing it? > > Making the atom feed work would really be a new feature, since as far as I > can tell they were never actually implemented. Not sure about that, looking at this POC, it seems that it has been broken by the move to Template::Toolkit
This report needs further clarification. What is the exact purpose of the POC patch? Do we want to get rid of (unfinished?) atom format or on the other hand add support for it?
The purpose of the POC is to prove it could be fixed quickly. I think the atom feeds have worked before but have been broken when migrating to TT.
Would it be reasonable to fix the RSS here and then move the atom discussion/fix/removal to another bug report so this one can get through?
(In reply to Nick Clemens from comment #13) > Would it be reasonable to fix the RSS here and then move the atom > discussion/fix/removal to another bug report so this one can get through? Yes! Sounds like the Atom feed is something that should be addressed in another bug. I'm all for moving this along. Christopher
Do we need to decide here if we want to support ATOM or RSS as a standard? We have had reports about the RSS feed not working and I'd like to help get things moving here again. Patches currently don't apply.
Created attachment 76148 [details] [review] Bug 16111 [Revised] RSS feed for OPAC search results has wrong content type This patch fixing handling of OPAC search results feeds by correcting parameters passed to the script and making format checks consistent in the script. This patch also removes altogether references to a separate "atom" formatted feed, because the template for that feed displays no content (and never has). To test, apply the patch and perform a search in the OPAC. - Click the RSS link and confirm that the browser displays a feed with no errors. In Chrome the feed should display a "document tree" view. - Using the browser's developer console, verify that the content-type of the document is defined as "text/xml." - Validate the resulting feed at https://validator.w3.org/feed/ and confirm that it is valid. Revision adds suggestion from QA to accept the old 'rss2' parameter and treat it like an 'rss' parameter. Also changed: Corrected another place where 'rss2' was used in the template. Signed-off-by: Magnus Enger <magnus@librietech.no> Before the patch: - RSS is served with content-type text/html - The link from the RSS icon ona search page looks like opac-search.pl?idx=kw&q=perl&count=50&sort_by=acqdate_dsc&format=rss2 After applying the patch: - RSS is served as text/xml - The RSS link leads to opac-search.pl?idx=kw&q=perl&count=50&sort_by=acqdate_dsc&format=rss But the old link with format=rss2 still works
I am sorry but we are doing something very wrong here. The POC patch shows that it is very easy to support atom, whereas the "Revised" patch is removing all this code. I would suggest to: 1. Fix the content type for rss 2. Fix the rss2 vs rss and keep support for rss2 3. Add the support for atom
Created attachment 76160 [details] [review] Bug 16111: Fix content type for RSS feed Test plan: GET http://catalogue/cgi-bin/koha/opac-search.pl?idx=kw&q=perl&count=50&sort_by=acqdate_dsc&format=rss2 => Without this patch it returns content type text/html => With this patch it returns text/xml GET http://catalogue/cgi-bin/koha/opac-search.pl?idx=kw&q=perl&count=50&sort_by=acqdate_dsc&format=rss => Without this patch it does not work => With this patch it returns text/xml
Created attachment 76161 [details] [review] Bug 16111: Fix atom support
Created attachment 76162 [details] [review] Bug 16111: Remove 'rss2' which should be 'rss' or 'atom'
Do whatever you want, but my opinion would be to not remove code that could be fixed easily. To fix the original issue, it's a 3-lines patch ( Bug 16111: Fix content type for RSS feed ).
Owen, could we get your opinion on Jonathan's follow-up? Patches don't apply right now as attached to the bug, but I am not sure if only some should be applied. Please take a look.
(In reply to Katrin Fischer from comment #22) > Owen, could we get your opinion on Jonathan's follow-up? > Patches don't apply right now as attached to the bug, but I am not sure if > only some should be applied. Please take a look. They apply correctly, it's either Owen's patch (the first one), or mines (the 3 others).
Created attachment 77057 [details] [review] Bug 16111: Fix content type for RSS feed Test plan: GET http://catalogue/cgi-bin/koha/opac-search.pl?idx=kw&q=perl&count=50&sort_by=acqdate_dsc&format=rss2 => Without this patch it returns content type text/html => With this patch it returns text/xml GET http://catalogue/cgi-bin/koha/opac-search.pl?idx=kw&q=perl&count=50&sort_by=acqdate_dsc&format=rss => Without this patch it does not work => With this patch it returns text/xml Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 77058 [details] [review] Bug 16111: Fix atom support Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 77059 [details] [review] Bug 16111: Remove 'rss2' which should be 'rss' or 'atom' Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 77060 [details] [review] Bug 16111: (follow-up) Fixes for simplicity and validation This patch simplifies the OPAC search results feed template by adding BLOCKs for a couple of repeated sections. The same is done for the feed URL in the search results template. This patch also makes some changes to the markup of the rss and Atom feeds to improve their validity. To test, apply the patch and perform a search in the OPAC. Clicking the RSS icon at the top of the search results page should bring up the RSS feed, which should render correctly. If you validate the source it should return only a couple of "recommendations:" - "Self reference doesn't match document location" - "Unregistered link relationship: search" Perform the same test with the Atom feed by modifying the URL of the RSS feed in your browser so that it ends in "format=atom" instead of "format=rss" The Atom feed has some more serious validation issues but should be functional.
Patch failed to apply to Biblibre Sandbox 1 -- Some problems occurred applying patches from bug 16111: <h1>Something went wrong !</h1>Applying: Bug 16111: Fix content type for RSS feed Applying: Bug 16111: Fix atom support Using index info to reconstruct a base tree... M koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-opensearch.tt Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-opensearch.tt CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-opensearch.tt error: Failed to merge in the changes. Patch failed at 0001 Bug 16111: Fix atom support The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run git bz apply --continue. If you would prefer to skip this patch, instead run git bz apply --skip. To restore the original branch and stop patching run git bz apply --abort. Bug 16111 - RSS feed for OPAC search results has wrong content type 77057 - Bug 16111: Fix content type for RSS feed 77058 - Bug 16111: Fix atom support 77059 - Bug 16111: Remove 'rss2' which should be 'rss' or 'atom' 77060 - Bug 16111: (follow-up) Fixes for simplicity and validation Apply? [(y)es, (n)o, (i)nteractive] Patch left in /tmp/Bug-16111-Fix-atom-support-69Epde.patch .
Created attachment 78578 [details] [review] Bug 16111: Fix content type for RSS feed Test plan: GET http://catalogue/cgi-bin/koha/opac-search.pl?idx=kw&q=perl&count=50&sort_by=acqdate_dsc&format=rss2 => Without this patch it returns content type text/html => With this patch it returns text/xml GET http://catalogue/cgi-bin/koha/opac-search.pl?idx=kw&q=perl&count=50&sort_by=acqdate_dsc&format=rss => Without this patch it does not work => With this patch it returns text/xml Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 78579 [details] [review] Bug 16111: Fix atom support Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 78580 [details] [review] Bug 16111: Remove 'rss2' which should be 'rss' or 'atom' Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 78581 [details] [review] Bug 16111: (follow-up) Fixes for simplicity and validation This patch simplifies the OPAC search results feed template by adding BLOCKs for a couple of repeated sections. The same is done for the feed URL in the search results template. This patch also makes some changes to the markup of the rss and Atom feeds to improve their validity. To test, apply the patch and perform a search in the OPAC. Clicking the RSS icon at the top of the search results page should bring up the RSS feed, which should render correctly. If you validate the source it should return only a couple of "recommendations:" - "Self reference doesn't match document location" - "Unregistered link relationship: search" Perform the same test with the Atom feed by modifying the URL of the RSS feed in your browser so that it ends in "format=atom" instead of "format=rss" The Atom feed has some more serious validation issues but should be functional.
Comment on attachment 78579 [details] [review] Bug 16111: Fix atom support Review of attachment 78579 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-opensearch.tt @@ +93,5 @@ > > [% FOREACH SEARCH_RESULT IN SEARCH_RESULTS %] > <entry> > + <title>[% SEARCH_RESULT.title | html %] [% FOREACH subtitl IN SEARCH_RESULT.subtitle %], [% subtitl.subfield | html %][% END %]</title> > + <link>[% IF ( SEARCH_RESULT.BiblioDefaultViewmarc ) %][% OPACBaseURL | html %]/cgi-bin/koha/opac-MARCdetail.pl?biblionumber=[% SEARCH_RESULT.biblionumber | html %][% ELSE %][% IF ( SEARCH_RESULT.BiblioDefaultViewisbd ) %][% OPACBaseURL | html %]/cgi-bin/koha/opac-ISBDdetail.pl?biblionumber=[% SEARCH_RESULT.biblionumber | html %][% ELSE %][% OPACBaseURL | html %]/cgi-bin/koha/opac-detail.pl?biblionumber=[% SEARCH_RESULT.biblionumber | html %][% END %][% END %]</link> Shouldn't the URL be in the link's href attribute ?
(In reply to Julian Maurice from comment #33) > Comment on attachment 78579 [details] [review] [review] > Bug 16111: Fix atom support > > Review of attachment 78579 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-opensearch.tt > @@ +93,5 @@ > > > > [% FOREACH SEARCH_RESULT IN SEARCH_RESULTS %] > > <entry> > > + <title>[% SEARCH_RESULT.title | html %] [% FOREACH subtitl IN SEARCH_RESULT.subtitle %], [% subtitl.subfield | html %][% END %]</title> > > + <link>[% IF ( SEARCH_RESULT.BiblioDefaultViewmarc ) %][% OPACBaseURL | html %]/cgi-bin/koha/opac-MARCdetail.pl?biblionumber=[% SEARCH_RESULT.biblionumber | html %][% ELSE %][% IF ( SEARCH_RESULT.BiblioDefaultViewisbd ) %][% OPACBaseURL | html %]/cgi-bin/koha/opac-ISBDdetail.pl?biblionumber=[% SEARCH_RESULT.biblionumber | html %][% ELSE %][% OPACBaseURL | html %]/cgi-bin/koha/opac-detail.pl?biblionumber=[% SEARCH_RESULT.biblionumber | html %][% END %][% END %]</link> > > Shouldn't the URL be in the link's href attribute ? The <link> element is an empty element, it contains attributes only. So yes.
- <link href="[% OPACBaseURL | html %]/cgi-bin/koha/opac-search.pl?[% query_cgi |uri %][% limit_cgi |uri %]&format=rss2"/> - <updated>[% timestamp | html %]</updated> + <link href="[% OPACBaseURL %]/cgi-bin/koha/opac-search.pl?[% query_cgi |uri %][% limit_cgi |uri %]&format=atom"/> + <updated>[% timestamp %]</updated> rss2 => atom here Copy and paste ?
(In reply to Marcel de Rooy from comment #35) > - <link href="[% OPACBaseURL | html %]/cgi-bin/koha/opac-search.pl?[% > query_cgi |uri %][% limit_cgi |uri %]&format=rss2"/> > - <updated>[% timestamp | html %]</updated> > + <link href="[% OPACBaseURL %]/cgi-bin/koha/opac-search.pl?[% query_cgi > |uri %][% limit_cgi |uri %]&format=atom"/> > + <updated>[% timestamp %]</updated> > > rss2 => atom here > Copy and paste ? Nope, that's the point of these patches, "rss2" was wrong and should be "rss" or "atom".
Created attachment 78689 [details] [review] Bug 16111: Replace <link> with <link href...
Created attachment 81285 [details] [review] Bug 16111: Fix content type for RSS feed Test plan: GET http://catalogue/cgi-bin/koha/opac-search.pl?idx=kw&q=perl&count=50&sort_by=acqdate_dsc&format=rss2 => Without this patch it returns content type text/html => With this patch it returns text/xml GET http://catalogue/cgi-bin/koha/opac-search.pl?idx=kw&q=perl&count=50&sort_by=acqdate_dsc&format=rss => Without this patch it does not work => With this patch it returns text/xml Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Created attachment 81286 [details] [review] Bug 16111: Fix atom support Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Created attachment 81287 [details] [review] Bug 16111: Remove 'rss2' which should be 'rss' or 'atom' Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Created attachment 81288 [details] [review] Bug 16111: Replace <link> with <link href... Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Patch doesn't apply, can you please rebase? Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 16111: Fix content type for RSS feed Applying: Bug 16111: Fix atom support Applying: Bug 16111: Remove 'rss2' which should be 'rss' or 'atom' error: sha1 information is lacking or useless (koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-opensearch.tt). error: could not build fake ancestor
Jonathan could you take a stab at rebasing this? I got lost.
Created attachment 92050 [details] [review] Bug 16111: Fix content type for RSS feed Test plan: GET http://catalogue/cgi-bin/koha/opac-search.pl?idx=kw&q=perl&count=50&sort_by=acqdate_dsc&format=rss2 => Without this patch it returns content type text/html => With this patch it returns text/xml GET http://catalogue/cgi-bin/koha/opac-search.pl?idx=kw&q=perl&count=50&sort_by=acqdate_dsc&format=rss => Without this patch it does not work => With this patch it returns text/xml Bug 16111: Fix atom support Bug 16111: Remove 'rss2' which should be 'rss' or 'atom' Bug 16111: Replace <link> with <link href...
Patches squashed and SO-by lines removed. Please retest.
Is html for OpacBaseUrl correct? FAIL koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-opensearch.tt FAIL filters missing_filter at line 100 ( <link rel="alternate" href="[% OPACBaseURL | html %]/cgi-bin/koha/opac-search.pl?[% SEARCH_RESULT.query_cgi | $raw %][% SEARCH_RESULT.limit_cgi | $raw %]pw=[% page %]&format=atom" type="application/atom+xml"/>)
Created attachment 92362 [details] [review] Bug 16111: Adjust some more filters
(In reply to Katrin Fischer from comment #46) > Is html for OpacBaseUrl correct? Nope!
Still the same ;) FAIL koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-opensearch.tt FAIL filters missing_filter at line 100 ( <link rel="alternate" href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-search.pl?[% SEARCH_RESULT.query_cgi | $raw %][% SEARCH_RESULT.limit_cgi | $raw %]pw=[% page %]&format=atom" type="application/atom+xml"/>) I think we are missing [% page %]
Created attachment 92363 [details] [review] Bug 16111: Adjust some more filters
I fixed one issue with the <link> to: <link>[% OPACBaseURL | url %]/cgi-bin/koha/opac-search.pl?[% query_cgi | $raw %][% limit_cgi | $raw %]&format=rss</link> But I still get an XML error in the browser: XML Parsing Error: not well-formed Location: http://localhost:8080/cgi-bin/koha/opac-search.pl?idx=kw&q=perl&count=50&sort_by=acqdate_dsc&format=rss2 Line Number 17, Column 119: <atom:link rel="self" type="application/rss+xml" href="http://localhost:8080/cgi-bin/koha/opac-search.pl?idx=kw&q=perl&sort_by=acqdate_dsc&format=atom"/> The error points to the = before perl.
Wondering if bug 23378 is related/fixed by this.
Created attachment 92366 [details] [review] Bug 16111: Replace & with &
I've been using the Feed validator here. There are still 2 issues: https://validator.w3.org/feed/check.cgi - The link element in the item element is wrong with the href, it shoudl be <link>url</link> - There can't be elements between the item elements. I believe they go into the first part, before the item elements are listed and not repeated before each item element: <atom:link rel="search" type="application/opensearchdescription+xml" href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-search.pl?[% SEARCH_RESULT.query_cgi | $raw %][% SEARCH_RESULT.limit_cgi | $raw %]&sort_by=[% SEARCH_RESULT.sort_by |uri %]&format=opensearchdescription"/> <opensearch:Query role="request" searchTerms="[% SEARCH_RESULT.query_desc |uri %][% SEARCH_RESULT.limit_desc |uri %]" startPage="[% SEARCH_RESULT.page | html %]" /> Was checking here: https://github.com/dewitt/opensearch/blob/master/opensearch-1-1-draft-6.md#opensearch-response-elements
> > Shouldn't the URL be in the link's href attribute ? > > The <link> element is an empty element, it contains attributes only. > So yes. The validators don't like that and every RSS documentation I found it looks like this: <link>url</link>
Created attachment 92435 [details] [review] Bug 16111: Fix content type for RSS feed Test plan: GET http://catalogue/cgi-bin/koha/opac-search.pl?idx=kw&q=perl&count=50&sort_by=acqdate_dsc&format=rss2 => Without this patch it returns content type text/html => With this patch it returns text/xml GET http://catalogue/cgi-bin/koha/opac-search.pl?idx=kw&q=perl&count=50&sort_by=acqdate_dsc&format=rss => Without this patch it does not work => With this patch it returns text/xml Bug 16111: Fix atom support Bug 16111: Remove 'rss2' which should be 'rss' or 'atom' Bug 16111: Replace <link> with <link href... Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 92436 [details] [review] Bug 16111: Adjust some more filters Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 92437 [details] [review] Bug 16111: Replace & with & Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 92438 [details] [review] Bug 16111: Further fixes to the RSS to make feed work - fixed links per the WC3 spec - fixed a problem in the ATOM link To test: do a search, click the RSS icon - it shows xml in most browsers but wont do RSS type things. Apply this patch, restart the things refresh the page, it should do RSS things (i.e. firefox should render it as a nice looking thing, rather than raw XML.) noting that there are a couple of validation errors still, but they don't seem to be show stopping and moreover I wasn't sure how to fix them: Misplaced Item Self reference doesn't match document location (this is a "for widest compatibility you should fix" kind of thing)
Created attachment 92439 [details] [review] Bug 16111: Further fixes to the RSS to make feed work - fixed links per the WC3 spec - fixed a problem in the ATOM link To test: do a search, click the RSS icon - it shows xml in most browsers but wont do RSS type things. Apply this patch, restart the things refresh the page, it should do RSS things (i.e. firefox should render it as a nice looking thing, rather than raw XML.) noting that there are a couple of validation errors still, but they don't seem to be show stopping and moreover I wasn't sure how to fix them: Misplaced Item Self reference doesn't match document location (this is a "for widest compatibility you should fix" kind of thing) Signed-off-by: Owen Leonard <oleonard@myacpl.org>
*** Bug 23378 has been marked as a duplicate of this bug. ***
QA: Looking here now
> refresh the page, it should do RSS things (i.e. firefox should render it as a nice looking thing, rather than raw XML.) Is that actually the case? Since version 64, there is no more a feed reader to have a nice looking thing, an extension is required. https://support.mozilla.org/en-US/kb/feed-reader-replacements-firefox Thank Owen for linking with bug 23378, Will test this one as soon as possible.
I am seeing URLs like /cgi-bin/koha/opac-search.pl?&sort_by=relevance_dsc&format=atom OR /cgi-bin/koha/opac-search.pl?amp;idx=kw&q=perl&format=rss The extra ampersand was removed (before) by: $query_cgi =~ s/^&//; # remove unnecessary & from beginning of the query cgi But this is a side effect of the third patch. Note that I am afraid that this patch making changes to C4/Search.pm might break more things than we see now. And imo these changes are not needed for this purpose. I propose to partially revert that patch in QA. Soon submitting it.
The rss template contains things like: SEARCH_RESULT.query_cgi and SEARCH_RESULT.limit_cgi These are empty! SEARCH_RESULTS comes from: @newresults = searchResults( $search_context, $query_desc, $hits, $results_per_page, $offset, $scan, $results_hashref->{$server}->{"RECORDS"}); This sub does not return keys for query_cgi and limit_cgi. It does contain a result number: $oldbiblio->{result_number} = $i + 1; Will correct one obvious error. Remainder should be corrected elsewhere. It results in quite a few useless links.
opac-search.pl +$format = 'rss' if $format =~ /rss/; Hmm. Maybe we should only allow rss2 now too ? Adding a follow-up.
Should we use dc:creator or author? Koha just puts author in item:title element Specs: An item's title element holds character data that provides the item's headline. See also: For all elements defined in the RSS specification that enclose character data, the text SHOULD be interpreted as plain text with the exception of an item's description element, which MUST be suitable for presentation as HTML. All of these elements MUST NOT contain child elements. => Koha puts html inside in the title element Will not touch it here
Before each item we have: <atom:link rel="search" type="application/opensearchdescription+xml" href="/cgi-bin/koha/opac-search.pl?&sort_by=&format=opensearchdescription"/> NO ITEM INFO ? <opensearch:Query role="request" searchTerms="" startPage=""/> EMPTY search term and page ! Seems useless. Not touching it here
Git grep on rss2 koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-showreviews-rss.tt: <link>[% OPACBaseURL | html %]/cgi-bin/koha/opac-showreviews.pl&format=rss2</link> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-showreviews-rss.tt: <atom:link href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-showreviews.pl&format=rss2" rel="self" type="application/rss+xml" /> => Script opac-showreviews does only work with format=rss !! opac/unapi: 'rss2' => 'MARC21slim2RSS2.xsl', opac/unapi: 'rss2-full' => 'MARC21slim2RSS2.xsl', opac/unapi: 'rss2' => 'MARC21slim2RSS2.xsl', opac/unapi: 'rss2-full' => 'MARC21slim2RSS2.xsl', opac/unapi: 'rss2' => q(<format name="rss2" type="application/xml"/>), opac/unapi: 'rss2-full' => q(<format name="rss2-full" type="application/xml"/>), debian/templates/koha-conf-site.xml.in: <retrieval syntax="xml" name="rss2"> debian/templates/koha-conf-site.xml.in: <retrieval syntax="xml" name="rss2">
Created attachment 92618 [details] [review] Bug 16111: Fix content type for RSS feed Test plan: GET http://catalogue/cgi-bin/koha/opac-search.pl?idx=kw&q=perl&count=50&sort_by=acqdate_dsc&format=rss2 => Without this patch it returns content type text/html => With this patch it returns text/xml GET http://catalogue/cgi-bin/koha/opac-search.pl?idx=kw&q=perl&count=50&sort_by=acqdate_dsc&format=rss => Without this patch it does not work => With this patch it returns text/xml Bug 16111: Fix atom support Bug 16111: Remove 'rss2' which should be 'rss' or 'atom' Bug 16111: Replace <link> with <link href... Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 92619 [details] [review] Bug 16111: Adjust some more filters Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 92620 [details] [review] Bug 16111: [CHANGED] Replace link href by link in rss part Original patch title: Replace & with & Signed-off-by: Liz Rea <wizzyrea@gmail.com> EDIT: Reverting the changes to C4/Search and opac-search.pl. This requires a few changes, as supplied in the 5th patch. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 92621 [details] [review] Bug 16111: Further fixes to the RSS to make feed work - fixed links per the WC3 spec - fixed a problem in the ATOM link To test: do a search, click the RSS icon - it shows xml in most browsers but wont do RSS type things. Apply this patch, restart the things refresh the page, it should do RSS things (i.e. firefox should render it as a nice looking thing, rather than raw XML.) noting that there are a couple of validation errors still, but they don't seem to be show stopping and moreover I wasn't sure how to fix them: Misplaced Item Self reference doesn't match document location (this is a "for widest compatibility you should fix" kind of thing) Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 92622 [details] [review] Bug 16111: (QA follow-up) Changes related to partial revert When reverting the & change, we need to go back to html filter for query_cgi and limit_cgi. In this patch I only replaced it where it is needed. The template contains more references to SEARCH_RESULT.query_cgi and SEARCH_RESULT.limit_cgi which are useless, since searchResults does not return these hash keys at all. This patch fixes one occurrence where SEARCH_RESULT was copied outside the loop that defines the template variable. Obviously, the code for RSS still needs more attention. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 92623 [details] [review] Bug 16111: (QA follow-up) Few additional corrections [1] Refining the regex on the format parameter in opac-search.pl [2] Adding a colon to dc:identifier. The Dublin Core specs gives me this example: Identifier="ISBN:0385424728" [3] Replacing last occurrence of rss2 in the rss template
Created attachment 92624 [details] [review] Bug 16111: (QA follow-up) Few additional corrections [1] Refining the regex on the format parameter in opac-search.pl [2] Adding a colon to dc:identifier. The Dublin Core specs gives me this example: Identifier="ISBN:0385424728" [3] Replacing last occurrence of rss2 in the rss template Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
@RM: RSS needs further attention but I think this is a good start now. See earlier comments. I reverted some changes in the third patch. You might want to ask Jonathan for comments. But I would not pass QA on the original proposal.
Nice work! Pushed to master for 19.11.00
Pushed to 19.05.x for 19.05.04
@Lucas I'm trying to backport to 18.11, it's not trivial but it seems safe. Will link a branch after finishing and testing it.
Hi, here is a branch with the backported patches from 19.05.x on top of current 18.11.x https://git.biblibre.com/biblibre/kohac/commits/branch/18.11.x-backport-bz-16111 I tested the RSS feed on the OPAC, the validator shows the same non fatal errors as on master. @Lucas There were few conflicts so double check in case I missed something but they were all almost the same so it's actually simpler than it looked initially.
backported to 18.11.x for 18.11.11 thanks for helping with merge conflicts Victor!