Bug 29785 - Koha::Object->messages must be renamed
Summary: Koha::Object->messages must be renamed
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 29230
  Show dependency treegraph
 
Reported: 2022-01-04 13:27 UTC by Jonathan Druart
Modified: 2022-12-12 21:23 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.03,21.05.11


Attachments
Bug 29785: Rename Koha::Object->message with ->object_messages (10.90 KB, patch)
2022-01-04 13:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29785: Rename Koha::Object->message with ->object_messages (10.96 KB, patch)
2022-01-31 16:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29785: Rename Koha::Object->message with ->object_messages (11.05 KB, patch)
2022-02-01 14:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2022-01-04 13:27:58 UTC
It will conflict with other ->messages methods, it's too generic.

Bug 29230 needs Koha::Patron->messages to return Koha::Patron::Messages for instance.
Comment 1 Jonathan Druart 2022-01-04 13:38:13 UTC
Created attachment 128986 [details] [review]
Bug 29785: Rename Koha::Object->message with ->object_messages

It will conflict with other ->messages methods, it's too generic.

Bug 29230 needs Koha::Patron->messages to return Koha::Patron::Messages for instance.

Test plan:
Confirm that the tests modified by this patch still pass
Comment 2 Tomás Cohen Arazi 2022-01-31 16:22:27 UTC
Created attachment 130018 [details] [review]
Bug 29785: Rename Koha::Object->message with ->object_messages

It will conflict with other ->messages methods, it's too generic.

Bug 29230 needs Koha::Patron->messages to return Koha::Patron::Messages for instance.

Test plan:
Confirm that the tests modified by this patch still pass

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi 2022-01-31 16:23:03 UTC
Rebased and signed. Trivial change, 'messages()' was cleary too generic. Good catch, Jonathan!
Comment 4 Marcel de Rooy 2022-02-01 12:24:35 UTC
This looks like it still needs attention though:

=head3 messages

    my @messages = @{ $bool->messages };

Returns the I<Koha::Object::Message> objects that were recorded.

=cut

sub messages {
    my ( $self ) = @_;

    $self->{_messages} = []
        unless defined $self->{_messages};

    return $self->{_messages};
}

Kind of strange btw that we add a lot of code there?
Koha::Result::Boolean->new(0)->add_message({ message => $error });

Please provide feedback! Thanks
Comment 5 Jonathan Druart 2022-02-01 13:51:25 UTC
(In reply to Marcel de Rooy from comment #4)
> This looks like it still needs attention though:
> 
> =head3 messages
> 
>     my @messages = @{ $bool->messages };
> 
> Returns the I<Koha::Object::Message> objects that were recorded.
> 
> =cut
> 
> sub messages {
>     my ( $self ) = @_;
> 
>     $self->{_messages} = []
>         unless defined $self->{_messages};
> 
>     return $self->{_messages};
> }

This looks correct to me, Koha::Result::Boolean is not inheriting from Koha::Object.

> Kind of strange btw that we add a lot of code there?
> Koha::Result::Boolean->new(0)->add_message({ message => $error });

Can you detail what you mean here?
Comment 6 Marcel de Rooy 2022-02-01 14:02:32 UTC
(In reply to Jonathan Druart from comment #5)
> (In reply to Marcel de Rooy from comment #4)
> This looks correct to me, Koha::Result::Boolean is not inheriting from
> Koha::Object.

Okay

> > Kind of strange btw that we add a lot of code there?
> > Koha::Result::Boolean->new(0)->add_message({ message => $error });
> 
> Can you detail what you mean here?

Forget that. Result here confused me a bit. Surely it not a Schema::Result, but what is a Koha::Result actually?
No answer needed here :)
Comment 7 Marcel de Rooy 2022-02-01 14:05:47 UTC
Created attachment 130043 [details] [review]
Bug 29785: Rename Koha::Object->message with ->object_messages

It will conflict with other ->messages methods, it's too generic.

Bug 29230 needs Koha::Patron->messages to return Koha::Patron::Messages for instance.

Test plan:
Confirm that the tests modified by this patch still pass

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Fridolin Somers 2022-02-10 08:03:25 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 9 Fridolin Somers 2022-02-10 08:06:27 UTC
Should we also rename Koha::Object::add_message ?
Comment 10 Kyle M Hall 2022-02-11 11:40:10 UTC
Pushed to 21.11.x for 21.11.03
Comment 11 Andrew Fuerste-Henry 2022-02-21 16:38:27 UTC
Pushed to 21.05.x for 21.05.11
Comment 12 Victor Grousset/tuxayo 2022-02-24 23:38:08 UTC
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.