Bug 18539 - Forbid Koha::Objects->find calls in list context
Summary: Forbid Koha::Objects->find calls in list context
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 18179
Blocks: 19809 18361 19695
  Show dependency treegraph
 
Reported: 2017-05-04 14:08 UTC by Jonathan Druart
Modified: 2017-12-14 08:18 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18539: Forbid list context calls for Koha::Objects->find (2.57 KB, patch)
2017-05-04 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18539: Forbid list context calls for Koha::Objects->find (2.62 KB, patch)
2017-06-15 15:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18539: remove occurrence in ReNewSubscription.t (2.37 KB, patch)
2017-06-15 18:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18539: remove occurrence in ReNewSubscription.t (1.22 KB, patch)
2017-08-03 13:35 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2017-05-04 14:08:39 UTC
See bug 18179 for more info.
Comment 1 Jonathan Druart 2017-05-04 14:11:20 UTC
Created attachment 63110 [details] [review]
Bug 18539: Forbid list context calls for Koha::Objects->find

Reading https://perlmaven.com/how-to-return-undef-from-a-function
this sound like the more correct behaviour.

Considering:
$template->param(
    stuff => Koha::Stuffs->find( $id ),
    foo   => 1,
);
without this patch, if the $id does not represent any rows in the DB,
stuff will be assigned to 'foo' and $foo will be undef in the template.
That can lead to very bad side-effects.

With this patch we make sure that it will never happen again.

Test plan:
  prove t/db_dependent/Koha/Objects.t
should return green

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 2 Kyle M Hall 2017-06-15 15:46:01 UTC
Created attachment 64338 [details] [review]
Bug 18539: Forbid list context calls for Koha::Objects->find

Reading https://perlmaven.com/how-to-return-undef-from-a-function
this sound like the more correct behaviour.

Considering:
$template->param(
    stuff => Koha::Stuffs->find( $id ),
    foo   => 1,
);
without this patch, if the $id does not represent any rows in the DB,
stuff will be assigned to 'foo' and $foo will be undef in the template.
That can lead to very bad side-effects.

With this patch we make sure that it will never happen again.

Test plan:
  prove t/db_dependent/Koha/Objects.t
should return green

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 3 Jonathan Druart 2017-06-15 18:46:40 UTC
Created attachment 64351 [details] [review]
Bug 18539: remove occurrence in ReNewSubscription.t
Comment 4 Jonathan Druart 2017-06-15 19:08:58 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 5 Fridolin SOMERS 2017-06-20 08:53:15 UTC
Enhancement not pushed to 17.05.x
Comment 6 Fridolin SOMERS 2017-08-03 07:40:42 UTC
Actual patch Bug 18539: remove occurrence in ReNewSubscription.t contains the remove of t/db_dependent/00-strict.t.

But I see this was not pushed into master :
http://git.koha-community.org/gitweb/?p=koha.git;a=commit;h=ad7ff09b995e5275f2c6d664b31351321b1674af
Comment 7 Fridolin SOMERS 2017-08-03 07:44:58 UTC
I finally to push to 17.05.x to be able to push Bug 18361.

Will be in 17.05.03.
Comment 8 Jonathan Druart 2017-08-03 13:35:03 UTC
Created attachment 65458 [details] [review]
Bug 18539: remove occurrence in ReNewSubscription.t

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 9 Katrin Fischer 2017-08-05 12:45:50 UTC
These patches have been pushed to 16.11.x and will be in 16.11.11.