[Bug 43663] New: Add Koha::Objects::Mixin::Polymorphic for polymorphic subclass dispatch
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43663 Bug ID: 43663 Summary: Add Koha::Objects::Mixin::Polymorphic for polymorphic subclass dispatch Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: martin.renvoize@openfifth.co.uk QA Contact: testopia@bugs.koha-community.org Target Milestone: --- Several Koha::Objects plural classes independently implement the same pattern: given a row, decide which Koha::Object subclass should represent it, based on the value of one column (a hand-rolled _polymorphic_field/_polymorphic_map pair plus an ~8-line object_class method). Koha::File::Transports (already in main) dispatches on the 'transport' column to Koha::File::Transport::{SFTP,FTP,Local}. Koha::Auth::Identity::Providers (bug 24880, not yet merged) dispatches on 'protocol' to Koha::Auth::Identity::Provider::{OAuth,OIDC,SAML2}. The same need came up again during review of bug 42870 (pluggable email transports). Koha::Auth::Identity::Provider::OAuth additionally hand-types its own dispatch key a second time, independently, inside its own new() ($params->{protocol} = 'OAuth';) - nothing ties that string to the _polymorphic_map entry in the plural class that has to match it, so the two can silently drift apart. This bug extracts the shared behaviour into a proper Koha::Objects::Mixin::* (the same convention Koha already uses for Koha::Objects::Mixin::AdditionalFields, composed via 'use parent qw( Koha::Objects Koha::Objects::Mixin::Foo )', used by 20+ plural classes), and migrates the one already-in-main consumer (Koha::File::Transports) onto it as proof it is behaviour-preserving. A consuming plural class declares: sub _polymorphic_field { return 'transport' } sub _polymorphic_classes { return (List::Of::Class::Names) } sub _polymorphic_base_class { return 'Fallback::Class::Name' } Each subclass self-declares its own dispatch key: sub _polymorphic_key { return 'sftp' } The mixin builds _polymorphic_map from _polymorphic_classes plus each class's own _polymorphic_key, and provides object_class() itself, so the copy-pasted dispatch logic disappears from every consumer, not just the map. A plural class needing genuinely custom mapping logic can still override _polymorphic_map (or object_class) directly - ordinary Perl inheritance makes that a free escape hatch. This is a proposal filed ahead of code, based on a design worked out with the QA team. It is deliberately narrow: it introduces no mechanism for Koha Plugins to register additional polymorphic subclasses, no Plugin management UI changes, and no koha-conf.xml changes - that is a separate, later concern once a concrete capability needs it. It does not touch Koha::Auth::Identity::Providers (bug 24880) or bug 42870 directly; both could adopt this mixin as a follow-up once merged. Patches to follow. Test plan (once patches are attached): 1. Apply the patch(es). 2. Run prove t/db_dependent/Koha/Objects/Mixin/Polymorphic.t and confirm all tests pass. 3. Run prove t/db_dependent/Koha/File/Transport.t t/db_dependent/Koha/File/Transports.t t/db_dependent/Koha/File/Transport/FTP.t t/db_dependent/Koha/File/Transport/Local.t t/db_dependent/Koha/File/Transport/SFTP.t and confirm they all still pass unmodified against the migrated Koha::File::Transports, proving the migration is behaviour-preserving. -- 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=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 ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |martin.renvoize@openfifth.c |ity.org |o.uk -- 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=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 ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- 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 #1 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 206952 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206952&action=edit Bug 43663: Add Koha::Objects::Mixin::Polymorphic 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 _polymorphic_field and _polymorphic_classes (an explicit, reviewed list - no namespace scanning or auto-discovery); each polymorphic subclass self-declares its own dispatch key via _polymorphic_key, so that key only ever needs to be written once. A consumer needing genuinely custom mapping logic can still override _polymorphic_map directly - ordinary Perl method resolution makes that a free escape hatch. A third method, _polymorphic_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 _polymorphic_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::Polymorphic )`, 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->_polymorphic_object_class(@_) }` - documented as mandatory in the mixin's own POD, not an optional convenience. Adds t/db_dependent/Koha/Objects/Mixin/Polymorphic.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 here are self-contained: they reuse the real file_transports table and the real, pre-existing Koha::File::Transport base class (both untouched by this commit), composed with small test-only subclasses/plural classes defined in the test file itself. TestBuilder's build_object() can't be used for these, since it tries to load the class from disk by path; build() plus a direct find() against the fixture class is used instead. Covers: the map built from _polymorphic_classes/_polymorphic_key, per-row dispatch, the inferred _polymorphic_base_class default (both with no object and for an unrecognised value), and that overriding _polymorphic_map directly in a consumer is still honoured. 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 #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 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #206952|0 |1 is obsolete| | Attachment #206953|0 |1 is obsolete| | -- 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 --- Comment #4 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 206963 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206963&action=edit Bug 43663: Migrate Koha::File::Transports onto the STI 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::SingleTableInheritance: - Koha::File::Transports now declares _sti_classes (an explicit, reviewed list - unchanged), and composes the mixin via `use parent`. It doesn't declare _sti_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 _sti_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 _sti_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/SingleTableInheritance.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. t/db_dependent/TestBuilder.t's generic class scan (updated in the previous commit) also continues to cover this class correctly. 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. 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 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 --- Comment #5 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- So.. I decided 'Polymorphic' wasn't the right term and opted for STI (SingleTableInheritance) as used in Rails/ActiveRecord frameworks for this sort of Polymorphism. -- 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43663 Olivier Hubert <olivier.hubert@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43663 Olivier Hubert <olivier.hubert@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #206963|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43663 --- Comment #6 from Olivier Hubert <olivier.hubert@inlibro.com> --- Created attachment 208673 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208673&action=edit Bug 43663: Migrate Koha::File::Transports onto the STI 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::SingleTableInheritance: - Koha::File::Transports now declares _sti_classes (an explicit, reviewed list - unchanged), and composes the mixin via `use parent`. It doesn't declare _sti_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 _sti_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 _sti_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/SingleTableInheritance.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. t/db_dependent/TestBuilder.t's generic class scan (updated in the previous commit) also continues to cover this class correctly. 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. 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> Signed-off-by: Olivier Hubert <olivier.hubert@inlibro.com> -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org