Description
Katrin Fischer
2014-07-12 11:00:43 UTC
This is actually not a feature difference, but i just noticed that results display is much slower (twice as slow it seems) when going through xslt. Which is probably not a surprise, but maybe a good thing to mention here. I feel like the non-XSLT view was deprecated just not removed? (In reply to David Cook from comment #2) > I feel like the non-XSLT view was deprecated just not removed? I don't understand what you're saying. (In reply to Owen Leonard from comment #3) > (In reply to David Cook from comment #2) > > I feel like the non-XSLT view was deprecated just not removed? > > I don't understand what you're saying. Deprecated just means that it's not endorsed/recommended anymore but it is still available in the system. The XSLT view has been the default and the recommended view for quite a few years now. Even if we never explicitly said it was deprecated it has been deprecated in practice. If you look at ./koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt, there are about 152 lines of non-XSLT view. I think at this point that we could safely remove those lines. (In reply to David Cook from comment #4) > (In reply to Owen Leonard from comment #3) > > (In reply to David Cook from comment #2) > > > I feel like the non-XSLT view was deprecated just not removed? > > > > I don't understand what you're saying. > > Deprecated just means that it's not endorsed/recommended anymore but it is > still available in the system. The XSLT view has been the default and the > recommended view for quite a few years now. Even if we never explicitly said > it was deprecated it has been deprecated in practice. > > If you look at ./koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt, > there are about 152 lines of non-XSLT view. I think at this point that we > could safely remove those lines. We did officially deprecate it, check the coding guidelines: https://wiki.koha-community.org/wiki/Coding_Guidelines#DEPR1:_non-XSLT_search_results_and_bib_display_templates_deprecated The problems are features not implemented for the non-XSLT view that have been considered as blockers for final removal. Created attachment 122141 [details] [review] Bug 12561: Remove HighlightOwnItemsOnOPAC and HighlightOwnItemsOnOPACWhich This is the only remaining non-XSLT only feature. We should remove the non-XSLT views that have been deprecated for a while now, and remove this feature. Created attachment 122142 [details] [review] Bug 12561: Remove non-XSLT views Created attachment 122143 [details] [review] Bug 12561: Change prototype of XSLTParse4Display Use a hash for better readability and reusability. Created attachment 122144 [details] [review] Bug 12561: Add warning on about page If a default xslt file has been removed or if the one filled in one of the syspref does not exist. After 7 years, we are going to remove them, yeah \o/ Test plan: Play with the different sysprefs and confirm that: - empty or "default" means the same => the default xslt file is used - Have a look at the about page, if you are pointing to a file that has been removed you should see a warning there QA will take care of catching inconsistencies in the code, and eventually code that was used for the non-XSLT views only and must be removed. Sorry Jonathan.. I get 'sha1 information is lacking or useless' on the third patch. They look good but I can't SO without testing ;) Created attachment 122600 [details] [review] Bug 12561: Change prototype of XSLTParse4Display Use a hash for better readability and reusability. Created attachment 122601 [details] [review] Bug 12561: Add warning on about page If a default xslt file has been removed or if the one filled in one of the syspref does not exist. Created attachment 122607 [details] [review] Bug 12561: Remove HighlightOwnItemsOnOPAC and HighlightOwnItemsOnOPACWhich This is the only remaining non-XSLT only feature. We should remove the non-XSLT views that have been deprecated for a while now, and remove this feature. Created attachment 122608 [details] [review] Bug 12561: Remove non-XSLT views Created attachment 122609 [details] [review] Bug 12561: Change prototype of XSLTParse4Display Use a hash for better readability and reusability. Created attachment 122610 [details] [review] Bug 12561: Add warning on about page If a default xslt file has been removed or if the one filled in one of the syspref does not exist. Created attachment 123262 [details] [review] Bug 12561: Remove HighlightOwnItemsOnOPAC and HighlightOwnItemsOnOPACWhich This is the only remaining non-XSLT only feature. We should remove the non-XSLT views that have been deprecated for a while now, and remove this feature. Created attachment 123263 [details] [review] Bug 12561: Remove non-XSLT views Created attachment 123264 [details] [review] Bug 12561: Change prototype of XSLTParse4Display Use a hash for better readability and reusability. Created attachment 123265 [details] [review] Bug 12561: Add warning on about page If a default xslt file has been removed or if the one filled in one of the syspref does not exist. Created attachment 123267 [details] [review] Bug 12561: Remove HighlightOwnItemsOnOPAC and HighlightOwnItemsOnOPACWhich This is the only remaining non-XSLT only feature. We should remove the non-XSLT views that have been deprecated for a while now, and remove this feature. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 123268 [details] [review] Bug 12561: Remove non-XSLT views Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 123269 [details] [review] Bug 12561: Change prototype of XSLTParse4Display Use a hash for better readability and reusability. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 123270 [details] [review] Bug 12561: Add warning on about page If a default xslt file has been removed or if the one filled in one of the syspref does not exist. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 123300 [details] [review] Bug 12561: Remove HighlightOwnItemsOnOPAC and HighlightOwnItemsOnOPACWhich This is the only remaining non-XSLT only feature. We should remove the non-XSLT views that have been deprecated for a while now, and remove this feature. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 123301 [details] [review] Bug 12561: Remove non-XSLT views Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 123302 [details] [review] Bug 12561: Change prototype of XSLTParse4Display Use a hash for better readability and reusability. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 123303 [details] [review] Bug 12561: Add warning on about page If a default xslt file has been removed or if the one filled in one of the syspref does not exist. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Nice job. There's a QA failure about POD. Tests still pass: $ prove t/db_dependent/Search.t \ t/db_dependent/XSLT.t \ t/XSLT.t \ t/db_dependent/UsageStats.t \ t/db_dependent/Auth.t Overall, the XSLTParse4Display method is well covered by tests so I'm not worried about the lack of tests on the splited subs. Created attachment 123305 [details] [review] Bug 12561: Remove HighlightOwnItemsOnOPAC and HighlightOwnItemsOnOPACWhich This is the only remaining non-XSLT only feature. We should remove the non-XSLT views that have been deprecated for a while now, and remove this feature. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 123306 [details] [review] Bug 12561: Remove non-XSLT views Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 123307 [details] [review] Bug 12561: Change prototype of XSLTParse4Display Use a hash for better readability and reusability. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 123308 [details] [review] Bug 12561: Add warning on about page If a default xslt file has been removed or if the one filled in one of the syspref does not exist. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> (In reply to Tomás Cohen Arazi from comment #31) > Nice job. There's a QA failure about POD. > > Tests still pass: > $ prove t/db_dependent/Search.t \ > t/db_dependent/XSLT.t \ > t/XSLT.t \ > t/db_dependent/UsageStats.t \ > t/db_dependent/Auth.t > > Overall, the XSLTParse4Display method is well covered by tests so I'm not > worried about the lack of tests on the splited subs. What he said :-) we overlapped Pushed to master for 21.11, thanks to everybody involved! |