Bug 25044 - No need to define koha_object[s]_class for standard object class names
Summary: No need to define koha_object[s]_class for standard object class names
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on: 24356
Blocks:
  Show dependency treegraph
 
Reported: 2020-04-02 14:02 UTC by Jonathan Druart
Modified: 2020-11-30 21:48 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 25044: Remove the need to define koha_object[s]_class for standard object class names (3.72 KB, patch)
2020-04-02 14:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25044: Remove the need to define koha_object[s]_class for standard object class names (3.25 KB, patch)
2020-04-02 14:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25044: Remove the need to define koha_object[s]_class for standard object class names (3.30 KB, patch)
2020-04-08 04:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25044: Remove the need to define koha_object[s]_class for standard object class names (3.36 KB, patch)
2020-04-12 19:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25044: (follow-up) Fix wrong tests (904 bytes, patch)
2020-04-14 11:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2020-04-02 14:02:01 UTC
In Koha::Object[s]->prefetch_whitelist, there is a call to the koha_object[s]_class DBIC resultset class. We should not, otherwise those 2 methods will have to be defined all the time, even when we can guess it easily.

Koha::Item <> Schema::Result::Item => standard
Koha::Acquisition::Order <> Schema::Result::Aqorder => non-standard

sub _get_object_class {
    my ( $type ) = @_; 
    return unless $type;

    if( $type->can('koha_object_class') ) { 
        return $type->koha_object_class;
    }   
    $type =~ s|Schema::Result::||;
    return ${type};
}
Comment 1 Jonathan Druart 2020-04-02 14:04:41 UTC
Created attachment 102297 [details] [review]
Bug 25044: Remove the need to define koha_object[s]_class for standard object class names

In Koha::Object[s]->prefetch_whitelist, there is a call to the koha_object[s]_class DBIC resultset class. We should not, otherwise those 2 methods will have to be defined all the time, even when we can guess it easily.

Koha::Item <> Schema::Result::Item => standard
Koha::Acquisition::Order <> Schema::Result::Aqorder => non-standard

sub _get_object_class {
    my ( $type ) = @_;
    return unless $type;

    if( $type->can('koha_object_class') ) {
        return $type->koha_object_class;
    }
    $type =~ s|Schema::Result::||;
    return ${type};
}

Test plan:
 % prove t/db_dependent/Koha/Object.t t/db_dependent/Koha/Objects.t
should return green before and after this patch
Comment 2 Tomás Cohen Arazi 2020-04-02 14:11:22 UTC
Good catch, I think we missed that method. Still, wouldn't it be more efficient to just have this information as schema metadata? (not even a method, just metadata in the big schema hash)
Comment 3 Jonathan Druart 2020-04-02 14:21:30 UTC
Created attachment 102298 [details] [review]
Bug 25044: Remove the need to define koha_object[s]_class for standard object class names

In Koha::Object[s]->prefetch_whitelist, there is a call to the koha_object[s]_class DBIC resultset class. We should not, otherwise those 2 methods will have to be defined all the time, even when we can guess it easily.

Koha::Item <> Schema::Result::Item => standard
Koha::Acquisition::Order <> Schema::Result::Aqorder => non-standard

sub _get_object_class {
    my ( $type ) = @_;
    return unless $type;

    if( $type->can('koha_object_class') ) {
        return $type->koha_object_class;
    }
    $type =~ s|Schema::Result::||;
    return ${type};
}

Test plan:
 % prove t/db_dependent/Koha/Object.t t/db_dependent/Koha/Objects.t
should return green before and after this patch
Comment 4 Tomás Cohen Arazi 2020-04-08 04:19:34 UTC
Created attachment 102529 [details] [review]
Bug 25044: Remove the need to define koha_object[s]_class for standard object class names

In Koha::Object[s]->prefetch_whitelist, there is a call to the koha_object[s]_class DBIC resultset class. We should not, otherwise those 2 methods will have to be defined all the time, even when we can guess it easily.

Koha::Item <> Schema::Result::Item => standard
Koha::Acquisition::Order <> Schema::Result::Aqorder => non-standard

sub _get_object_class {
    my ( $type ) = @_;
    return unless $type;

    if( $type->can('koha_object_class') ) {
        return $type->koha_object_class;
    }
    $type =~ s|Schema::Result::||;
    return ${type};
}

Test plan:
 % prove t/db_dependent/Koha/Object.t t/db_dependent/Koha/Objects.t
should return green before and after this patch

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Katrin Fischer 2020-04-12 19:28:47 UTC
Created attachment 102774 [details] [review]
Bug 25044: Remove the need to define koha_object[s]_class for standard object class names

In Koha::Object[s]->prefetch_whitelist, there is a call to the koha_object[s]_class DBIC resultset class. We should not, otherwise those 2 methods will have to be defined all the time, even when we can guess it easily.

Koha::Item <> Schema::Result::Item => standard
Koha::Acquisition::Order <> Schema::Result::Aqorder => non-standard

sub _get_object_class {
    my ( $type ) = @_;
    return unless $type;

    if( $type->can('koha_object_class') ) {
        return $type->koha_object_class;
    }
    $type =~ s|Schema::Result::||;
    return ${type};
}

Test plan:
 % prove t/db_dependent/Koha/Object.t t/db_dependent/Koha/Objects.t
should return green before and after this patch

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 6 Katrin Fischer 2020-04-12 19:29:40 UTC
Trusting Tomas and Jonathan here as experts - not a big code change and tests pass.
Comment 7 Tomás Cohen Arazi 2020-04-13 17:32:31 UTC
(In reply to Katrin Fischer from comment #6)
> Trusting Tomas and Jonathan here as experts - not a big code change and
> tests pass.

Yeah, we are just 'doing it right' on something Agustin and I overlooked a bit.
Comment 8 Martin Renvoize 2020-04-14 07:37:29 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 9 Jonathan Druart 2020-04-14 09:36:07 UTC
Tomas, one test is failing:

t/Koha/REST/Plugin/Query.t .. 1/6
    #   Failed test 'exact match for JSON Pointer "/prefetch/0"'
    #   at t/Koha/REST/Plugin/Query.t line 350.
    #     Structures begin differing at:
    #          $got = HASH(0x55daaa8747c8)
    #     $expected = 'biblio'
    # Looks like you failed 1 test of 10.

I do not understand what was supposed to do the code prior to this change.

Koha::Schema::Result::Biblio does not have a koha_object_class, so the try was failing, and undef was assigned.

I am suspecting the tests to be wrong, but it would be good if you or Agustin could confirm it.
Comment 10 Tomás Cohen Arazi 2020-04-14 11:58:51 UTC
Created attachment 102927 [details] [review]
Bug 25044: (follow-up) Fix wrong tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Martin Renvoize 2020-04-14 14:24:03 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 12 Joy Nelson 2020-05-05 20:42:31 UTC
missing dependencies.  not backported to 19.11.x