Bug 16111 - RSS feed for OPAC search results has wrong content type
Summary: RSS feed for OPAC search results has wrong content type
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
: 23378 (view as bug list)
Depends on:
Blocks: 21332
  Show dependency treegraph
 
Reported: 2016-03-21 15:29 UTC by Galen Charlton
Modified: 2020-06-04 20:34 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.04,18.11.11


Attachments
Bug 16111 - RSS feed for OPAC search results has wrong content type (10.40 KB, patch)
2016-03-21 17:59 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 16111 [Revised] RSS feed for OPAC search results has wrong content type (11.71 KB, patch)
2016-03-25 12:55 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 16111 [Revised] RSS feed for OPAC search results has wrong content type (12.10 KB, patch)
2016-03-29 11:53 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 16111: POC (5.07 KB, patch)
2016-04-04 07:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16111 [Revised] RSS feed for OPAC search results has wrong content type (12.10 KB, patch)
2018-06-18 18:54 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 16111: Fix content type for RSS feed (2.03 KB, patch)
2018-06-18 20:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16111: Fix atom support (5.08 KB, patch)
2018-06-18 20:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16111: Remove 'rss2' which should be 'rss' or 'atom' (9.14 KB, patch)
2018-06-18 20:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16111: Fix content type for RSS feed (2.07 KB, patch)
2018-07-17 16:05 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 16111: Fix atom support (5.12 KB, patch)
2018-07-17 16:05 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 16111: Remove 'rss2' which should be 'rss' or 'atom' (9.17 KB, patch)
2018-07-17 16:05 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 16111: (follow-up) Fixes for simplicity and validation (19.36 KB, patch)
2018-07-17 16:58 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 16111: Fix content type for RSS feed (2.07 KB, patch)
2018-09-11 21:11 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 16111: Fix atom support (6.77 KB, patch)
2018-09-11 21:12 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 16111: Remove 'rss2' which should be 'rss' or 'atom' (9.51 KB, patch)
2018-09-11 21:12 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 16111: (follow-up) Fixes for simplicity and validation (24.86 KB, patch)
2018-09-11 21:12 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 16111: Replace <link> with <link href... (3.70 KB, patch)
2018-09-14 18:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16111: Fix content type for RSS feed (2.14 KB, patch)
2018-10-26 13:22 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 16111: Fix atom support (6.84 KB, patch)
2018-10-26 13:22 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 16111: Remove 'rss2' which should be 'rss' or 'atom' (9.59 KB, patch)
2018-10-26 13:23 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 16111: Replace <link> with <link href... (3.77 KB, patch)
2018-10-26 13:23 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 16111: Fix content type for RSS feed (17.17 KB, patch)
2019-08-07 15:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16111: Adjust some more filters (10.03 KB, patch)
2019-08-19 20:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16111: Adjust some more filters (10.03 KB, patch)
2019-08-19 21:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16111: Replace & with &amp; (4.19 KB, patch)
2019-08-19 22:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16111: Fix content type for RSS feed (17.22 KB, patch)
2019-08-22 14:38 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 16111: Adjust some more filters (10.08 KB, patch)
2019-08-22 14:38 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 16111: Replace & with &amp; (4.23 KB, patch)
2019-08-22 14:38 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 16111: Further fixes to the RSS to make feed work (3.59 KB, patch)
2019-08-22 15:10 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 16111: Further fixes to the RSS to make feed work (3.65 KB, patch)
2019-08-22 15:52 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 16111: Fix content type for RSS feed (17.31 KB, patch)
2019-09-06 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16111: Adjust some more filters (10.17 KB, patch)
2019-09-06 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16111: [CHANGED] Replace link href by link in rss part (2.00 KB, patch)
2019-09-06 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16111: Further fixes to the RSS to make feed work (3.74 KB, patch)
2019-09-06 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16111: (QA follow-up) Changes related to partial revert (3.51 KB, patch)
2019-09-06 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16111: (QA follow-up) Few additional corrections (2.78 KB, patch)
2019-09-06 09:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16111: (QA follow-up) Few additional corrections (2.84 KB, patch)
2019-09-06 09:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Galen Charlton 2016-03-21 15:29:06 UTC
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.
Comment 1 Owen Leonard 2016-03-21 17:59:14 UTC Comment hidden (obsolete)
Comment 2 Galen Charlton 2016-03-22 15:08:52 UTC
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/;
Comment 3 Owen Leonard 2016-03-25 12:55:29 UTC Comment hidden (obsolete)
Comment 4 Magnus Enger 2016-03-29 07:33:53 UTC
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.
Comment 5 Magnus Enger 2016-03-29 11:46:39 UTC
Please disregard comment #4, I was not testing with English as the chosen language. Guess I am a bit rusty...
Comment 6 Magnus Enger 2016-03-29 11:53:30 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2016-03-31 12:27:10 UTC
Would not it better to fix the atom feeds instead of removing it?
Comment 8 Owen Leonard 2016-04-02 15:16:07 UTC
(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.
Comment 9 Jonathan Druart 2016-04-04 07:17:11 UTC
Created attachment 49837 [details] [review]
Bug 16111: POC
Comment 10 Jonathan Druart 2016-04-04 07:18:41 UTC
(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
Comment 11 Marcel de Rooy 2016-04-22 07:33:56 UTC
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?
Comment 12 Jonathan Druart 2016-04-22 20:55:47 UTC
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.
Comment 13 Nick Clemens 2016-05-26 20:33:47 UTC
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?
Comment 14 Christopher Brannon 2016-05-26 21:02:48 UTC
(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
Comment 15 Katrin Fischer 2018-04-15 11:45:22 UTC
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.
Comment 16 Owen Leonard 2018-06-18 18:54:14 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2018-06-18 20:31:07 UTC
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
Comment 18 Jonathan Druart 2018-06-18 20:51:33 UTC
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
Comment 19 Jonathan Druart 2018-06-18 20:51:39 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2018-06-18 20:51:45 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2018-06-18 20:53:20 UTC
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 ).
Comment 22 Katrin Fischer 2018-07-01 20:51:24 UTC
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.
Comment 23 Jonathan Druart 2018-07-02 13:50:06 UTC
(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).
Comment 24 Owen Leonard 2018-07-17 16:05:03 UTC Comment hidden (obsolete)
Comment 25 Owen Leonard 2018-07-17 16:05:07 UTC Comment hidden (obsolete)
Comment 26 Owen Leonard 2018-07-17 16:05:12 UTC Comment hidden (obsolete)
Comment 27 Owen Leonard 2018-07-17 16:58:02 UTC Comment hidden (obsolete)
Comment 28 Cab Vinton 2018-08-29 00:05:03 UTC
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 .
Comment 29 Owen Leonard 2018-09-11 21:11:58 UTC
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>
Comment 30 Owen Leonard 2018-09-11 21:12:03 UTC
Created attachment 78579 [details] [review]
Bug 16111: Fix atom support

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 31 Owen Leonard 2018-09-11 21:12:08 UTC
Created attachment 78580 [details] [review]
Bug 16111: Remove 'rss2' which should be 'rss' or 'atom'

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 32 Owen Leonard 2018-09-11 21:12:12 UTC Comment hidden (obsolete)
Comment 33 Julian Maurice 2018-09-14 09:31:10 UTC
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 ?
Comment 34 Marcel de Rooy 2018-09-14 10:16:58 UTC
(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.
Comment 35 Marcel de Rooy 2018-09-14 10:17:58 UTC
-    <link href="[% OPACBaseURL | html %]/cgi-bin/koha/opac-search.pl?[% query_cgi |uri %][% limit_cgi |uri %]&amp;format=rss2"/>
-   <updated>[% timestamp | html %]</updated>
+    <link href="[% OPACBaseURL %]/cgi-bin/koha/opac-search.pl?[% query_cgi |uri %][% limit_cgi |uri %]&amp;format=atom"/>
+   <updated>[% timestamp %]</updated>

rss2 => atom here
Copy and paste ?
Comment 36 Jonathan Druart 2018-09-14 18:37:44 UTC
(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 %]&amp;format=rss2"/>
> -   <updated>[% timestamp | html %]</updated>
> +    <link href="[% OPACBaseURL %]/cgi-bin/koha/opac-search.pl?[% query_cgi
> |uri %][% limit_cgi |uri %]&amp;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".
Comment 37 Jonathan Druart 2018-09-14 18:47:31 UTC
Created attachment 78689 [details] [review]
Bug 16111: Replace <link> with <link href...
Comment 38 Andrew Isherwood 2018-10-26 13:22:54 UTC
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>
Comment 39 Andrew Isherwood 2018-10-26 13:22:59 UTC
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>
Comment 40 Andrew Isherwood 2018-10-26 13:23:04 UTC
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>
Comment 41 Andrew Isherwood 2018-10-26 13:23:09 UTC
Created attachment 81288 [details] [review]
Bug 16111: Replace <link> with <link href...

Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Comment 42 Katrin Fischer 2018-10-27 20:33:34 UTC
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
Comment 43 Owen Leonard 2019-08-07 14:47:52 UTC
Jonathan could you take a stab at rebasing this? I got lost.
Comment 44 Jonathan Druart 2019-08-07 15:09:22 UTC
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...
Comment 45 Jonathan Druart 2019-08-07 15:10:02 UTC
Patches squashed and SO-by lines removed. Please retest.
Comment 46 Katrin Fischer 2019-08-17 19:42:26 UTC
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 %]&amp;format=atom" type="application/atom+xml"/>)
Comment 47 Jonathan Druart 2019-08-19 20:34:00 UTC
Created attachment 92362 [details] [review]
Bug 16111: Adjust some more filters
Comment 48 Jonathan Druart 2019-08-19 20:34:30 UTC
(In reply to Katrin Fischer from comment #46)
> Is html for OpacBaseUrl correct?

Nope!
Comment 49 Katrin Fischer 2019-08-19 21:12:32 UTC
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 %]&amp;format=atom" type="application/atom+xml"/>)


I think we are missing [% page %]
Comment 50 Jonathan Druart 2019-08-19 21:15:31 UTC
Created attachment 92363 [details] [review]
Bug 16111: Adjust some more filters
Comment 51 Katrin Fischer 2019-08-19 21:40:36 UTC
I fixed one issue with the <link> to:

     <link>[% OPACBaseURL | url %]/cgi-bin/koha/opac-search.pl?[% query_cgi | $raw %][% limit_cgi | $raw %]&amp;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&amp;sort_by=acqdate_dsc&amp;format=atom"/>

The error points to the = before perl.
Comment 52 Katrin Fischer 2019-08-19 21:41:50 UTC
Wondering if bug 23378 is related/fixed by this.
Comment 53 Jonathan Druart 2019-08-19 22:25:26 UTC
Created attachment 92366 [details] [review]
Bug 16111: Replace & with &amp;
Comment 54 Katrin Fischer 2019-08-20 06:38:38 UTC
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 %]&amp;sort_by=[% SEARCH_RESULT.sort_by |uri %]&amp;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
Comment 55 Katrin Fischer 2019-08-20 06:46:54 UTC
> > 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>
Comment 56 Liz Rea 2019-08-22 14:38:22 UTC
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>
Comment 57 Liz Rea 2019-08-22 14:38:34 UTC
Created attachment 92436 [details] [review]
Bug 16111: Adjust some more filters

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 58 Liz Rea 2019-08-22 14:38:39 UTC
Created attachment 92437 [details] [review]
Bug 16111: Replace & with &amp;

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 59 Liz Rea 2019-08-22 15:10:16 UTC
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)
Comment 60 Owen Leonard 2019-08-22 15:52:40 UTC
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>
Comment 61 Owen Leonard 2019-08-23 23:50:03 UTC
*** Bug 23378 has been marked as a duplicate of this bug. ***
Comment 62 Marcel de Rooy 2019-09-06 06:47:29 UTC
QA: Looking here now
Comment 63 Victor Grousset/tuxayo 2019-09-06 07:59:46 UTC
> 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.
Comment 64 Marcel de Rooy 2019-09-06 09:41:44 UTC
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.
Comment 65 Marcel de Rooy 2019-09-06 09:44:08 UTC
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.
Comment 66 Marcel de Rooy 2019-09-06 09:44:39 UTC
opac-search.pl
+$format = 'rss' if $format =~ /rss/;
Hmm. Maybe we should only allow rss2 now too ?
Adding a follow-up.
Comment 67 Marcel de Rooy 2019-09-06 09:50:33 UTC
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
Comment 68 Marcel de Rooy 2019-09-06 09:52:08 UTC
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
Comment 69 Marcel de Rooy 2019-09-06 09:55:16 UTC
Git grep on rss2

koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-showreviews-rss.tt:     <link>[% OPACBaseURL | html %]/cgi-bin/koha/opac-showreviews.pl&amp;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&amp;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">
Comment 70 Marcel de Rooy 2019-09-06 09:57:33 UTC
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>
Comment 71 Marcel de Rooy 2019-09-06 09:57:39 UTC
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>
Comment 72 Marcel de Rooy 2019-09-06 09:57:45 UTC
Created attachment 92620 [details] [review]
Bug 16111: [CHANGED] Replace link href by link in rss part

Original patch title: Replace & with &amp;

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>
Comment 73 Marcel de Rooy 2019-09-06 09:57:53 UTC
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>
Comment 74 Marcel de Rooy 2019-09-06 09:57:59 UTC
Created attachment 92622 [details] [review]
Bug 16111: (QA follow-up) Changes related to partial revert

When reverting the &amp; 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>
Comment 75 Marcel de Rooy 2019-09-06 09:58:06 UTC Comment hidden (obsolete)
Comment 76 Marcel de Rooy 2019-09-06 09:58:37 UTC
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>
Comment 77 Marcel de Rooy 2019-09-06 10:01:13 UTC
@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.
Comment 78 Martin Renvoize 2019-09-06 12:12:01 UTC
Nice work!

Pushed to master for 19.11.00
Comment 79 Fridolin Somers 2019-09-09 13:52:19 UTC
Pushed to 19.05.x for 19.05.04
Comment 80 Victor Grousset/tuxayo 2019-09-10 13:56:57 UTC
@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.
Comment 81 Victor Grousset/tuxayo 2019-09-10 19:32:03 UTC
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.
Comment 82 Lucas Gass 2019-10-02 08:04:45 UTC
backported to 18.11.x for 18.11.11

thanks for helping with merge conflicts Victor!