https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43663 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=42870 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43663 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@gmail.com, | |julian.maurice@biblibre.com | |, tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43663 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|NEW |Needs Signoff Sponsorship status|--- |Unsponsored -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43663 --- Comment #2 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 206953 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206953&action=edit Bug 43663: Migrate Koha::File::Transports onto the polymorphic mixin Koha::File::Transports already hand-rolled the same pattern the new mixin generalises: a static _polymorphic_map built by hand, with each subclass's dispatch key duplicated as a literal string both in that map and inside the subclass's own code. Migrates it and its three subclasses (SFTP, FTP, Local) onto Koha::Objects::Mixin::Polymorphic: - Koha::File::Transports now declares _polymorphic_classes (an explicit, reviewed list - unchanged), and composes the mixin via `use parent`. It doesn't declare _polymorphic_base_class: the mixin's default (inferred from Koha::Schema::Result::FileTransport's own koha_object_class) already resolves to the right answer, 'Koha::File::Transport'. - Each subclass self-declares its own dispatch key via _polymorphic_key, rather than that key living only in the plural class's map. - object_class is still defined directly on Koha::File::Transports, delegating to _polymorphic_object_class - see the mixin's own POD for why that one line can't just be inherited. The previous object_class used lc() on the transport value; dropped here as unnecessary, since both the admin template's <select> options and the REST swagger enum only ever produce lowercase values. No new test is added here: the mixin's own contract is already covered by t/db_dependent/Koha/Objects/Mixin/Polymorphic.t (previous commit), and the full existing File::Transport test suite (Transport.t, Transports.t, FTP.t, Local.t, SFTP.t) passes unmodified against this migration, confirming the mixin is a drop-in replacement for the hand-rolled version. This is an internal refactor with no intended behaviour change, so the test plan below is about confirming that: Administration > File transports should look and work exactly as it did before. Test plan: 1. Apply the patches and restart_all. 2. Go to Administration > File transports (/cgi-bin/koha/admin/file_transports.pl). If you already have any FTP/SFTP file transports configured, confirm the list still displays them correctly - before this fix, listing existing transports through the mixin failed outright with "Can't call method _new_from_dbic on an undefined value". 3. Click "New file transport", fill in the required fields, choose "FTP" as the Transport, and Save. - Confirm it appears in the list with Transport shown as "FTP". - Saving triggers an automatic connection test in the background; if you have a real FTP server to point it at, confirm the list eventually shows "Tests passing" for that row (or "Tests failing" with a sensible reason if the server is unreachable - either way, confirms an FTP-specific connection attempt actually ran, i.e. the row correctly dispatched to Koha::File::Transport::FTP). 4. Repeat step 3 choosing "SFTP" instead, confirming the row dispatches to Koha::File::Transport::SFTP. 5. Edit one of the transports created above (e.g. change the host) and Save again. Confirm the edit form is repopulated correctly and the change saves without error. 6. Delete one of the test transports created above and confirm it's removed from the list. Expected result throughout: no visible difference from the feature's behaviour prior to this patch. 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=43663 --- Comment #3 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 206962 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206962&action=edit Bug 43663: Add Koha::Objects::Mixin::SingleTableInheritance Three independent places in the Koha codebase have reinvented the same pattern - a Koha::Objects plural class that, given a row, decides which Koha::Object subclass should represent it, based on the value of one column: Koha::File::Transports (dispatching on 'transport'), Koha::Auth::Identity::Providers from bug 24880 (dispatching on 'protocol', not yet merged), and a variant discussed during the bug 42870 (pluggable email transports) review. Each reinvention is the same hand-rolled object_class method body and the same shape of _polymorphic_field/_polymorphic_map pair, with each subclass's own dispatch key typed out a second time by hand inside the subclass itself - nothing ties that string to the plural class's map entry it has to match, so the two can silently drift. This extracts the shared behaviour into a proper Koha::Objects::Mixin::* (a convention Koha already has and uses elsewhere, e.g. Koha::Objects::Mixin::AdditionalFields). A consuming plural class declares _sti_field and _sti_classes (an explicit, reviewed list - no namespace scanning or auto-discovery); each STI subclass self-declares its own dispatch key via _sti_key, so that key only ever needs to be written once. A consumer needing genuinely custom mapping logic can still override _sti_map directly - ordinary Perl method resolution makes that a free escape hatch. Named for Single Table Inheritance, the term most ORMs (Rails/ ActiveRecord, Doctrine, Hibernate) use for exactly this pattern - one table, one discriminator column, several subclasses. Deliberately not called "polymorphic": in Rails/ActiveRecord specifically, "polymorphic" already names a different pattern (a foreign key that can point at more than one parent table), and reusing that word here would mislead anyone coming from that background. A third method, _sti_base_class (the class to fall back to when no object is passed, or its value matches no known subclass), has a working default: it's inferred from the koha_object_class declared on the plural class's own DBIC result class - the same "what's the default row class for this table" fact Koha::Object's own row-to-class resolution already relies on elsewhere, so it rarely needs restating here. A consumer whose desired fallback genuinely differs can still override it directly. The mixin's dispatch logic is provided as _sti_object_class, not object_class itself: Koha::Objects already defines an empty object_class stub, and composed via `use parent qw( Koha::Objects Koha::Objects::Mixin::SingleTableInheritance )`, Perl's default depth-first MRO searches all of Koha::Objects' own ancestry before ever considering the second parent/mixin, so that empty stub always wins silently. Every consuming plural class must therefore define one required line of its own: `sub object_class { return shift->_sti_object_class(@_) }` - documented as mandatory in the mixin's own POD, not an optional convenience. Adds t/db_dependent/Koha/Objects/Mixin/SingleTableInheritance.t - the mixin's own contract tests. Rather than depend on a real production consumer (Koha::File::Transports isn't migrated onto the mixin until the next commit), the fixtures are self-contained: small test-only subclasses/plural classes, defined in the test file itself, that reuse the real file_transports table and the real, pre-existing Koha::File::Transport base class (both untouched by this commit). TestBuilder's build_object() can't be used for these, since it tries to Module::Load the class from a .pm on disk; build() plus a direct find() against the fixture plural class is used instead. Covers: the map built from _sti_classes/_sti_key, per-row dispatch, the inferred _sti_base_class default (both with no object and for an unrecognised value), and that overriding _sti_map directly in a consumer is still honoured. Also updates t/db_dependent/TestBuilder.t's generic "Test all classes" scan, which detects STI/polymorphic classes across the whole codebase by checking for a hand-rolled _polymorphic_field/_polymorphic_map pair. Broadened to also recognise this mixin's _sti_field/_sti_map, so Koha::File::Transports (migrated in the next commit) stays covered by this net instead of silently falling through to the "regular class" branch and failing (ref() would be a specific subclass, but object_class() with no argument correctly returns the base class). 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=43663 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Add |Add |Koha::Objects::Mixin::Polym |Koha::Objects::Mixin::Singl |orphic for polymorphic |eTableInheritance for |subclass dispatch |polymorphic subclass | |dispatch -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43663 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |43666 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43666 [Bug 43666] Allow plugins to register additional Single Table Inheritance subclasses -- You are receiving this mail because: You are watching all bug changes.