[Bug 43546] New: Standardize command-line scripts on Getopt::Long::Descriptive for safer, more consistent option handling
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43546 Bug ID: 43546 Summary: Standardize command-line scripts on Getopt::Long::Descriptive for safer, more consistent option handling Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Command-line Utilities Assignee: koha-bugs@lists.koha-community.org Reporter: martin.renvoize@openfifth.co.uk QA Contact: testopia@bugs.koha-community.org CC: jake.deery@openfifth.co.uk, robin@catalyst.net.nz Target Milestone: --- Koha's command-line scripts (misc/cronjobs/*.pl and more broadly the roughly 150 scripts under misc/) use plain Getopt::Long, which has a structural gap: its = and : spec modifiers only mean "this flag needs a value if it's used" -- there is no way to express "this flag itself is mandatory." Scripts that need a genuinely required option have to bolt on their own ad hoc check after parsing, for example: GetOptions( 'h|hours=s' => \$hours ); unless ($hours) { print $usage; die "ERROR: No --hours (-h) option defined"; } A survey of misc/cronjobs/ (58 scripts, 43 using GetOptions) found 14 scripts doing this kind of manual required-option check, each with its own bespoke error text, none of it declared anywhere Getopt::Long itself understands. This is a reliability and maintainability gap: Reliability -- a required option silently omitted is only caught by whatever ad hoc check (if any) the script's author remembered to write, which is inconsistent across scripts, easy to miss when writing a new one, and the failure mode for missing checks is scripts running with undef or empty values instead of failing fast. That is particularly risky for unattended cron execution where nobody is watching stdout. Security/robustness -- consistent, declarative validation of required input reduces the chance of an unset or malformed option flowing further into a script (file paths, SQL parameters, shell arguments) before anything checks it. This is defense-in-depth rather than a specific known vulnerability. Maintainability/readability -- option name, description, type, and required-ness end up scattered across the GetOptions call, a separate POD block, and a separate manual check, all of which can drift out of sync. A declarative spec keeps them in one place. Modernization -- this is a small, low-risk step toward more structured, self-documenting CLI code, consistent with Koha's general direction of reducing boilerplate and inconsistency across similar scripts. Proposal: Adopt Getopt::Long::Descriptive (pure Perl, no XS, actively maintained, built directly on top of Getopt::Long -- see https://metacpan.org/pod/Getopt::Long::Descriptive) as the standard for new/converted scripts, using its native "required" constraint, for example: my ($opt, $usage) = describe_options( '%c %o', [ 'hours|h=i', "hours since item was returned", { required => 1 } ], ... ); This makes "mandatory option" an explicit, declarative property of the script rather than something reconstructed from Getopt::Long spec syntax or scattered ad hoc checks, and it auto-generates consistent --help/usage text from the same declaration, removing the separately-maintained pod2usage boilerplate found in roughly 32 of the surveyed scripts. Suggested scope (staged, not a single patch): 1. Add Getopt::Long::Descriptive to cpanfile. 2. Document the convention for how new/converted scripts should declare and parse options -- possibly formalized as a small helper alongside Koha::Script, which currently only handles locking/logging/userenv and has no opinion on option parsing. 3. Convert a small number of scripts as a proof of concept, chosen for having a genuinely mandatory option today (e.g. cart_to_shelf.pl's -h/--hours). 4. Convert the remainder opportunistically over time, not as a single mass patch -- this changes each script's help/error output, which is a real (if minor) behavior change worth reviewing script by script. One real-world motivating example: the third-party koha-plugin-crontab plugin (still a work in progress, not recommended for production use yet -- https://github.com/openfifth/koha-plugin-crontab) lets staff schedule these scripts from a UI and tries to surface which options are required in its form. Lacking any core-level signal, it has to either mis-infer this from Getopt::Long spec syntax or fall back to an administrator manually curating the list per install -- a workaround, not a fix. That is a symptom of the underlying gap this bug is about, not the primary motivation for it. Test plan: - For each converted script, confirm --help output is sensible and existing documented options still work. - Confirm omitting a required option now fails with a clear, consistent error before any side effects occur. - Confirm koha-qa.pl passes for converted scripts. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43546 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |martin.renvoize@openfifth.c |ity.org |o.uk CC| |andrew@bywatersolutions.com | |, nick@bywatersolutions.com -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43546 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |CONFIRMED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43546 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Sponsorship status|--- |Unsponsored Patch complexity|--- |Trivial patch Status|CONFIRMED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43546 --- Comment #1 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 206078 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206078&action=edit Bug 43546: Add Getopt::Long::Descriptive and convert cart_to_shelf.pl Koha's command-line scripts use plain Getopt::Long, which has no way to declare an option as genuinely mandatory -- scripts that need one bolt on their own ad hoc post-parse check, each with its own bespoke error text and none of it understood by Getopt::Long itself. This adds Getopt::Long::Descriptive to cpanfile and a thin Koha::Script->describe_options wrapper that establishes the convention for new/converted scripts: declare options (including required ones) declaratively, get a consistent --help and a consistent, fail-fast error for missing required options for free. Getopt::Long::Descriptive's usage_desc format string always renders the per-option description list immediately after itself, so there is no built-in slot for trailing "Examples:" text. describe_options accepts an optional trailing hashref with an 'epilog' key, printed after the option list, so more complex scripts than this proof of concept can still document usage examples. %c/%% are expanded in the epilog for consistency with the main format string. cart_to_shelf.pl is converted as a proof of concept, since its -h/--hours option is genuinely mandatory today and was enforced only by a manual "unless ($hours) { ... die }" check after parsing. Its short description and example, previously in a hand-rolled usage heredoc, are preserved via the new format string and epilog. This is a small, staged first step, not a mass conversion. The other cronjobs scripts are left on plain Getopt::Long for now. Test plan: 1. Update Perl dependencies (Getopt::Long::Descriptive is a new cpanfile requirement, already present on most systems as a transitive dependency, but confirm it's importable): perl -MGetopt::Long::Descriptive -e 1 2. Run: misc/cronjobs/cart_to_shelf.pl --help Confirm it prints a short description, the --hours/--help options, and a worked example -- and exits 0. 3. Run the script with no options: misc/cronjobs/cart_to_shelf.pl Confirm it fails immediately with a clear "Mandatory parameter 'hours' missing" error and the usage text, without querying the database, and exits non-zero. 4. Set an item's location to CART (e.g. via the staff interface or directly in the items table), then run: misc/cronjobs/cart_to_shelf.pl --hours 0 Confirm the item's location is reverted to its original shelving location, same as before this patch. 5. Confirm koha-qa.pl passes for the changed files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43546 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |43548 Blocks| |43549 Blocks| |43550 Blocks| |43551 Blocks| |43552 Blocks| |43553 Attachment #206078|0 |1 is obsolete| | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43548 [Bug 43548] Convert misc/cronjobs/runreport.pl to the Getopt::Long::Descriptive convention (see bug 43546) https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43549 [Bug 43549] Convert misc/cronjobs/cleanup_database.pl to the Getopt::Long::Descriptive convention (see bug 43546) https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43550 [Bug 43550] Convert misc/cronjobs/update_patrons_category.pl to Getopt::Long::Descriptive, with --from/--to as genuinely required options (see bug 43546) https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43551 [Bug 43551] Convert misc/cronjobs/process_message_queue.pl to the Getopt::Long::Descriptive convention (see bug 43546) https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43552 [Bug 43552] Convert misc/cronjobs/gather_print_notices.pl to the Getopt::Long::Descriptive convention (see bug 43546) https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43553 [Bug 43553] Convert misc/cronjobs/holds/holds_reminder.pl to the Getopt::Long::Descriptive convention (see bug 43546) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43546 --- Comment #2 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 206085 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206085&action=edit Bug 43546: Add Getopt::Long::Descriptive and convert cart_to_shelf.pl Koha's command-line scripts use plain Getopt::Long, which has no way to declare an option as genuinely mandatory -- scripts that need one bolt on their own ad hoc post-parse check, each with its own bespoke error text and none of it understood by Getopt::Long itself. This adds Getopt::Long::Descriptive to cpanfile and a thin Koha::Script->describe_options wrapper that establishes the convention for new/converted scripts: declare options (including required ones) declaratively, get a consistent --help and a consistent, fail-fast error for missing required options for free. Getopt::Long::Descriptive's usage_desc format string always renders the per-option description list immediately after itself, so there is no built-in slot for trailing "Examples:" text. describe_options accepts an optional trailing hashref with an 'epilog' key, printed after the option list, so more complex scripts than this proof of concept can still document usage examples. %c/%% are expanded in the epilog for consistency with the main format string. cart_to_shelf.pl is converted as a proof of concept, since its -h/--hours option is genuinely mandatory today and was enforced only by a manual "unless ($hours) { ... die }" check after parsing. Its short description and example, previously in a hand-rolled usage heredoc, are preserved via the new format string and epilog. This is a small, staged first step, not a mass conversion. The other cronjobs scripts are left on plain Getopt::Long for now. Test plan: 1. Update Perl dependencies (Getopt::Long::Descriptive is a new cpanfile requirement, already present on most systems as a transitive dependency, but confirm it's importable): perl -MGetopt::Long::Descriptive -e 1 2. Run: misc/cronjobs/cart_to_shelf.pl --help Confirm it prints a short description, the --hours/--help options, and a worked example -- and exits 0. 3. Run the script with no options: misc/cronjobs/cart_to_shelf.pl Confirm it fails immediately with a clear "Mandatory parameter 'hours' missing" error and the usage text, without querying the database, and exits non-zero. 4. Set an item's location to CART (e.g. via the staff interface or directly in the items table), then run: misc/cronjobs/cart_to_shelf.pl --hours 0 Confirm the item's location is reverted to its original shelving location, same as before this patch. 5. Confirm koha-qa.pl passes for the changed files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43546 --- Comment #3 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 206086 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206086&action=edit Bug 43546: (follow-up) Add --man and positional-argument support to Koha::Script->describe_options Converting the first real scripts to this convention (runreport.pl and update_patrons_category.pl, in dependent bugs) surfaced two gaps in what this bug set out to establish: - Scripts with substantial existing POD (like runreport.pl and update_patrons_category.pl) previously had --help (brief) and --man (full Pod::Usage manual), often with --help+--verbose also triggering the full manual. describe_options' own --help covers the brief case, but had no equivalent to --man. Rather than reinvent that per-script, --man is now added automatically alongside --help: it prints the calling script's own POD in full (Pod::Usage -verbose 2, defaulting to $0) and exits. --help+--verbose no longer triggers the full manual as a side effect; --verbose keeps its own, unrelated meaning of execution verbosity. Every script using describe_options gets --man for free. - A positional-argument requirement (e.g. runreport.pl's report ID(s)) has no per-option 'required' flag to declare it with, so it would otherwise be left as manual post-parse validation -- exactly the kind of ad hoc, unenforced, unparseable check this bug exists to replace for named options. describe_options now accepts a declarative 'args' key (in the same trailing hashref as 'epilog'), e.g. C<args => { min => 1, name => 'reportID', variadic => 1 }>, enforced by describe_options itself with the same consistent error/usage output as a missing required option. This also gives static tooling (e.g. the koha-plugin-crontab plugin, which reads script source rather than executing it) a literal marker for a required positional argument, the same way it already reads 'required => 1' for named options. describe_options also returns ($opt, $usage) in list context (unchanged in scalar context) for any post-parse validation 'args' doesn't cover. Test plan: 1. prove t/Koha/Script.t 2. koha-qa.pl passes for Koha/Script.pm (--man and args are exercised functionally by the scripts converted in the dependent bugs) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43546 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |43557 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43557 [Bug 43557] Add declarative mutually-exclusive option support to Koha::Script->describe_options -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43546 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|43551 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43551 [Bug 43551] Convert misc/cronjobs/process_message_queue.pl to the Getopt::Long::Descriptive convention (see bug 43546) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43546 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|43549 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43549 [Bug 43549] Convert misc/cronjobs/cleanup_database.pl to the Getopt::Long::Descriptive convention (see bug 43546) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43546 --- Comment #4 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 206186 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206186&action=edit Bug 43546: (follow-up) Reject negative --hours in cart_to_shelf.pl via a Params::Validate callback --hours is only checked for being present (required => 1), not for being a sensible value. A negative --hours would make the TIMESTAMPDIFF(HOUR, ...) > ? comparison true for essentially every item ever placed on the cart, reverting far more than intended, instead of failing with a clear error. Add a callback requiring --hours to be zero or a positive integer. Zero must stay valid: it's the documented immediate-revert case, already covered by this bug's own test plan (--hours 0 reverts cart items straight away), and is not itself the bug -- only negative values are. Test plan: 1. cart_to_shelf.pl --hours -5 Rejected: --hours must be zero or a positive integer. 2. cart_to_shelf.pl --hours 0 Still works exactly as before (immediate revert). 3. cart_to_shelf.pl --hours 24 Still works exactly as before. 4. koha-qa.pl passes. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43546 --- Comment #5 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 206187 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206187&action=edit Bug 43546: (follow-up) Require --hours to be a positive integer in cart_to_shelf.pl --hours was only checked for being present (required => 1), not for being a sensible value. The pre-conversion script used "unless ($hours) { die ... }", which treated 0 the same as not-supplied and rejected it; required => 1 only checks that the option was specified, so --hours 0 silently started passing instead of dying, and a negative --hours would make the TIMESTAMPDIFF(HOUR, ...) > ? comparison true for essentially every item ever placed on the cart, reverting far more than intended. Add a callback requiring --hours to be a positive integer, restoring the original rejection of 0 and closing the negative-number gap at the same time. TIMESTAMPDIFF(HOUR, ...) truncates to whole hours in MySQL, so there's no finer-grained value (e.g. 0.5) this query could ever act on regardless of type. Test plan: 1. cart_to_shelf.pl --hours 0 Rejected: --hours must be a positive integer (matches the pre-conversion script's behaviour). 2. cart_to_shelf.pl --hours -5 Rejected: --hours must be a positive integer. 3. cart_to_shelf.pl --hours 24 Still works exactly as before. 4. koha-qa.pl passes. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43546 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #206186|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org