I propose we add a wrapper class on top of `Text::CSV` that takes care of applying all the required settings we might need. It should provide a way to override those settings (e.g. separator character, formula, etc).
Created attachment 191466 [details] [review] Bug 41619: Add Koha::CSV This patch introduces Koha::CSV, a wrapper around Text::CSV_XS that provides consistent defaults and methods for CSV generation across Koha. The class: - Inherits CSV delimiter from CSVDelimiter system preference - Enforces security defaults (binary=1, formula='empty') - Provides convenient methods for common CSV operations - Allows per-instance configuration overrides This establishes a foundation for standardizing CSV exports throughout Koha, eliminating inconsistencies in CSV generation and providing a testable, maintainable approach to CSV handling. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/Koha/CSV.t => SUCCESS: Tests pass! 3. Tests cover: - Default initialization with CSVDelimiter preference - Preference value inheritance (comma, semicolon, tabulation) - Constructor parameter overrides - add_row() method with various data types - combine() and string() methods - print() to filehandle - Proper quote escaping and empty field handling 4. Sign off :-D
I used AI to analyze which `Text::CSV` methods we use across the codebase, and so required being added to `Koha::CSV`. Forgot to mention in the commit.
Created attachment 191491 [details] [review] Bug 41619: Add Koha::CSV This patch introduces Koha::CSV, a wrapper around Text::CSV_XS that provides consistent defaults and methods for CSV generation across Koha. The class: - Inherits CSV delimiter from CSVDelimiter system preference - Enforces security defaults (binary=1, formula='empty') - Provides convenient methods for common CSV operations - Allows per-instance configuration overrides This establishes a foundation for standardizing CSV exports throughout Koha, eliminating inconsistencies in CSV generation and providing a testable, maintainable approach to CSV handling. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/Koha/CSV.t => SUCCESS: Tests pass! 3. Tests cover: - Default initialization with CSVDelimiter preference - Preference value inheritance (comma, semicolon, tabulation) - Constructor parameter overrides - add_row() method with various data types - combine() and string() methods - print() to filehandle - Proper quote escaping and empty field handling 4. Sign off :-D Signed-off-by: David Nind <david@davidnind.com>
Created attachment 191551 [details] [review] Bug 41619: Add Koha::CSV This patch introduces Koha::CSV, a wrapper around Text::CSV_XS that provides consistent defaults and methods for CSV generation across Koha. The class: - Inherits CSV delimiter from CSVDelimiter system preference - Enforces security defaults (binary=1, formula='empty') - Provides convenient methods for common CSV operations - Allows per-instance configuration overrides This establishes a foundation for standardizing CSV exports throughout Koha, eliminating inconsistencies in CSV generation and providing a testable, maintainable approach to CSV handling. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/Koha/CSV.t => SUCCESS: Tests pass! 3. Tests cover: - Default initialization with CSVDelimiter preference - Preference value inheritance (comma, semicolon, tabulation) - Constructor parameter overrides - add_row() method with various data types - combine() and string() methods - print() to filehandle - Proper quote escaping and empty field handling 4. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Nice work everyone! Pushed to main for 26.05
xt/author/Text_CSV_Various.t .. 1/23 # Files using Text::CSV without formula protection: # Koha/CSV.pm # # CSV formula injection protection is required for security. # Add 'formula => "empty"' to Text::CSV constructor options. # Valid formula modes: 'empty' (recommended), 'die', 'croak', 'diag', or numeric 1-5 # Failed test 'All Text::CSV usage includes formula injection protection' # at xt/author/Text_CSV_Various.t line 157. # got: '1' # expected: '0' # Looks like you failed 1 test of 2.
Created attachment 193461 [details] [review] Bug 41619: (Follow-up) Consider Logical Defined-Or in test This patch fixes xt/author/Text_CSV_Various.t by using a regex that can consider logical defined-or statements (|| and //, etc) when checking which formula has been used. TO TEST: a) run prove -vv xt/author/Text_CSV_Various.t *) notice the test fails == APPLY PATCH == b) repeat step a) *) notice the test now passes c) inspect diff for xt/author/Text_CSV_Various.t *) notice we are now using a wildcard (.*) between => and the first ' to skip variable declarations d) inspect diff for Koha/CSV.pm *) notice we are now hard-coding 'empty' as a defined-or in case someone decides not to define a format at the class level (which was not being done before anyway and is now defaulted to undefined) == SIGN OFF ==
I've been getting this, too. I've resolved it by tweaking the regex and moving the hard-coded 'empty' to the Text::CSV_XS call. Others may have a different idea of how to resolve this, but my work is attached nonetheless. Ta, Jake (D)
Created attachment 193939 [details] [review] Bug 41619: (Follow-up) Consider Logical Defined-Or in test This patch fixes xt/author/Text_CSV_Various.t by using a regex that can consider logical defined-or statements (|| and //, etc) when checking which formula has been used. TO TEST: a) run prove -vv xt/author/Text_CSV_Various.t *) notice the test fails == APPLY PATCH == b) repeat step a) *) notice the test now passes c) inspect diff for xt/author/Text_CSV_Various.t *) notice we are now using a wildcard (.*) between => and the first ' to skip variable declarations d) inspect diff for Koha/CSV.pm *) notice we are now hard-coding 'empty' as a defined-or in case someone decides not to define a format at the class level (which was not being done before anyway and is now defaulted to undefined) == SIGN OFF == Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Follow-up pushed to main. Thanks Jake!
The follow up broke /kohadevbox/koha/t/Koha/CSV.t
Created attachment 194099 [details] [review] Bug 41619: (Follow-up) Fix for t/Koha/CSV.t This patch fixes an omission that caused t/Koha/CSV.t to fail. To test, prove t/Koha/CSV.t and notice how it now passes.
additional follow-up pushed to main. Thanks for the quick follow-up Jake!