Bug 18179 - Koha::Objects->find should not be called in list context
Summary: Koha::Objects->find should not be called in list context
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 18492 18510 18511 (view as bug list)
Depends on:
Blocks: 18510 18511 18539
  Show dependency treegraph
 
Reported: 2017-02-27 22:59 UTC by Liz Rea
Modified: 2017-06-26 08:05 UTC (History)
10 users (show)

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


Attachments
Bug 18179: Patron Restrictions not showing on member detail page (1.39 KB, patch)
2017-04-17 15:12 UTC, Marc Véron
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18179: Patron Restrictions not showing on member detail page (1.45 KB, patch)
2017-04-17 15:36 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 18179: Forbid list context calls for Koha::Objects->find (2.38 KB, patch)
2017-04-18 17:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18179: Update existing calls (3.40 KB, patch)
2017-04-18 17:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18179: Forbid list context calls for Koha::Objects->find (2.51 KB, patch)
2017-04-18 18:41 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18179: Update existing calls (3.59 KB, patch)
2017-04-18 18:51 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18179: Forbid list context calls for Koha::Objects->find (2.57 KB, patch)
2017-04-25 10:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18179: Update existing calls (3.63 KB, patch)
2017-04-25 10:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18179: Update 1 occurrence in booksellers.pl (992 bytes, patch)
2017-04-28 20:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18179: Update 1 occurrence in moremember.pl (1.72 KB, patch)
2017-04-28 20:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18179: Update existing calls (3.70 KB, patch)
2017-05-04 14:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18179: Update 1 occurrence in booksellers.pl (1.04 KB, patch)
2017-05-04 14:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18179: Update 1 occurrence in moremember.pl (1.79 KB, patch)
2017-05-04 14:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18179: Use borrower.category_type in patron-title.inc (1.42 KB, patch)
2017-05-04 14:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
[16.11.x] Bug 18179: Update existing calls (2.69 KB, patch)
2017-05-29 18:10 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 Liz Rea 2017-02-27 22:59:38 UTC
When a borrower has a restriction applied, the Restrictions tab on the Details screen doesn't show the restriction is there even though there's a message at the top of the screen saying the account is restricted.

This does appear correctly on the circulation page.
Comment 1 Marc Véron 2017-04-17 12:43:59 UTC
Confirmed for current master.
Comment 2 Marc Véron 2017-04-17 15:03:41 UTC
Hours of debugging later, one line of code messes up template params.
Comment 3 Marc Véron 2017-04-17 15:12:50 UTC
Created attachment 62216 [details] [review]
Bug 18179: Patron Restrictions not showing on member detail page

When a borrower has a restriction applied, the Restrictions tab on the Details
screen (moremember.pl) doesn't show the restriction is there even though there's a message at
the top of the screen saying the account is restricted.
The restrictions  appear correctly on the circulation page (circulation.pl).

To test:
- Apply patch
- Verify that restrictions tab on moremember.pl behaves the same as on
  circulation.pl
Comment 4 Marc Véron 2017-04-17 15:18:21 UTC
BTW: Syspref DumpTemplateVarsIntranet finally helped to find out that template variables were messed up. housebound_role got the variable name of the next template variable as value and all following template variables were wrong.
Comment 5 Owen Leonard 2017-04-17 15:36:24 UTC
Created attachment 62218 [details] [review]
[SIGNED-OFF] Bug 18179: Patron Restrictions not showing on member detail page

When a borrower has a restriction applied, the Restrictions tab on the Details
screen (moremember.pl) doesn't show the restriction is there even though there's a message at
the top of the screen saying the account is restricted.
The restrictions  appear correctly on the circulation page (circulation.pl).

To test:
- Apply patch
- Verify that restrictions tab on moremember.pl behaves the same as on
  circulation.pl

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 6 Jonathan Druart 2017-04-18 15:46:18 UTC
Yes we have a big problem here, it may be needed to fix it globally.
For instance:
in members/memberentry.pl

755 $template->param(
756     housebound_role  => Koha::Patron::HouseboundRoles->find($borrowernumber),
757 );

If someone add a param:

755 $template->param(
756     housebound_role  => Koha::Patron::HouseboundRoles->find($borrowernumber),
757     foo => 1,
758 );

housebound_role will be set to 'foo' if the patron does not have a housebound role, and foo will not be defined in the template.
Comment 7 Jonathan Druart 2017-04-18 16:48:49 UTC
Stealing this one for a more complete fix.
Comment 8 Jonathan Druart 2017-04-18 17:02:06 UTC
Created attachment 62304 [details] [review]
Bug 18179: 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
Comment 9 Jonathan Druart 2017-04-18 17:02:13 UTC
Created attachment 62305 [details] [review]
Bug 18179: Update existing calls

This patch updates the existing occurrences of ->find called in a list
context.
There are certainly others that are not easy to catch with git grep.
Test plan:
Confirm that the 4 modified scripts still works as expected.

We need this one ASAP in master to make sure we will not get other
side-effects of this kind and to catch possible uncaught occurrences
before the release.
Comment 10 Jonathan Druart 2017-04-18 17:03:04 UTC
@RMaints, I would not backport the first patch, but the second one is safe ("Update existing calls").
Comment 11 Marc Véron 2017-04-18 18:41:00 UTC
Created attachment 62319 [details] [review]
Bug 18179: 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>
Comment 12 Marc Véron 2017-04-18 18:51:14 UTC
Created attachment 62321 [details] [review]
Bug 18179: Update existing calls

This patch updates the existing occurrences of ->find called in a list
context.
There are certainly others that are not easy to catch with git grep.
Test plan:
Confirm that the 4 modified scripts still works as expected.

We need this one ASAP in master to make sure we will not get other
side-effects of this kind and to catch possible uncaught occurrences
before the release.

Tested scripts changed by this patch, they work as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 13 Nick Clemens 2017-04-25 10:45:01 UTC
Created attachment 62656 [details] [review]
Bug 18179: 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 14 Nick Clemens 2017-04-25 10:45:05 UTC
Created attachment 62657 [details] [review]
Bug 18179: Update existing calls

This patch updates the existing occurrences of ->find called in a list
context.
There are certainly others that are not easy to catch with git grep.
Test plan:
Confirm that the 4 modified scripts still works as expected.

We need this one ASAP in master to make sure we will not get other
side-effects of this kind and to catch possible uncaught occurrences
before the release.

Tested scripts changed by this patch, they work as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 15 Jonathan Druart 2017-04-25 13:32:38 UTC
*** Bug 18492 has been marked as a duplicate of this bug. ***
Comment 16 Kyle M Hall 2017-04-28 10:48:48 UTC
Pushed to master for 17.05, thanks Jonathan!
Comment 17 Kyle M Hall 2017-04-28 16:51:22 UTC
This patch is causing errors with many new find calls that have not been updated by the patch set. I think it would be best if we waited until post-release to push it back into master. That way we can find all the errors without subjecting librarians to them ; )
Comment 18 Jonathan Druart 2017-04-28 19:12:38 UTC
This patch should not have been reverted: Bug 18179: Update existing calls
It fixes the original issues.

Where do you get errors?
Comment 19 Jonathan Druart 2017-04-28 20:26:51 UTC
Created attachment 62879 [details] [review]
Bug 18179: Update 1 occurrence in booksellers.pl

To test:
1 - Load http://localhost:8081/cgi-bin/koha/acqui/booksellers.pl?booksellerid=1
2 - Should get internal server erro
3 - Apply patch
4 - Reload
5 - Should not get error
Comment 20 Jonathan Druart 2017-04-28 20:26:57 UTC
Created attachment 62880 [details] [review]
Bug 18179: Update 1 occurrence in moremember.pl

Cannot use "->find" in list context at /home/vagrant/kohaclone/members/moremember.pl line 334.

Test Plan:
1) Got to moremember.pl, note error
2) Apply this patch
3) Refresh page, no error!
Comment 21 Jonathan Druart 2017-04-28 20:27:06 UTC
*** Bug 18511 has been marked as a duplicate of this bug. ***
Comment 22 Jonathan Druart 2017-04-28 20:27:17 UTC
*** Bug 18510 has been marked as a duplicate of this bug. ***
Comment 23 Jonathan Druart 2017-04-28 20:28:40 UTC
This requires another QA.
Comment 24 Jonathan Druart 2017-04-28 20:30:25 UTC
Kyle, I think the last 3 patches should be pushed before the release.
We could move the croak to its own bug report if you do not feel confident with it.
Comment 25 Marcel de Rooy 2017-05-04 13:24:48 UTC
Note: 18361 also adds a subtest find.
Comment 26 Marcel de Rooy 2017-05-04 13:41:18 UTC
Same include:

[%- IF borrower.category.category_type == 'I' %]

[%- IF category_type == 'I' %]

I am having the feeling that it should be borrower.category_type when I am looking at GetMember.

Note that this change in the last patch could be considered out of scope here..
Also wondering about this hardcoded category too. I do not see in the sample patron categories. Please explain.
Comment 27 Marcel de Rooy 2017-05-04 13:48:22 UTC
Jonathan: If you move the first patch somewhere else and remove the category change, please update to Passed QA.
Comment 28 Jonathan Druart 2017-05-04 14:04:28 UTC
(In reply to Marcel de Rooy from comment #26)
> Same include:
> 
> [%- IF borrower.category.category_type == 'I' %]
> 
> [%- IF category_type == 'I' %]
> 
> I am having the feeling that it should be borrower.category_type when I am
> looking at GetMember.

Yes, right.
I found 3 occurrences where borrower is passed to the template:
members/pay.pl:        borrower => $borrower,
members/paycollect.pl:    borrower      => $borrower,
members/routing-lists.pl:    borrower          => $borrower,

$borrower comes from GetMember.

And from 1 members/moremember.pl (Koha::Patron)
So the change does not make sense.
But it's a regression of bug 12461, should be fixed elsewhere.

Please have a look at bug 17829 to get rid of all these issues!!
Comment 29 Jonathan Druart 2017-05-04 14:06:28 UTC
Created attachment 63106 [details] [review]
Bug 18179: Update existing calls

This patch updates the existing occurrences of ->find called in a list
context.
There are certainly others that are not easy to catch with git grep.
Test plan:
Confirm that the 4 modified scripts still works as expected.

We need this one ASAP in master to make sure we will not get other
side-effects of this kind and to catch possible uncaught occurrences
before the release.

Tested scripts changed by this patch, they work as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

bsoletes: 62657 - Bug 18179: Update existing calls
Comment 30 Jonathan Druart 2017-05-04 14:06:34 UTC
Created attachment 63107 [details] [review]
Bug 18179: Update 1 occurrence in booksellers.pl

To test:
1 - Load http://localhost:8081/cgi-bin/koha/acqui/booksellers.pl?booksellerid=1
2 - Should get internal server erro
3 - Apply patch
4 - Reload
5 - Should not get error

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 31 Jonathan Druart 2017-05-04 14:06:38 UTC
Created attachment 63108 [details] [review]
Bug 18179: Update 1 occurrence in moremember.pl

Cannot use "->find" in list context at /home/vagrant/kohaclone/members/moremember.pl line 334.

Test Plan:
1) Got to moremember.pl, note error
2) Apply this patch
3) Refresh page, no error!

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 32 Jonathan Druart 2017-05-04 14:06:43 UTC
Created attachment 63109 [details] [review]
Bug 18179: Use borrower.category_type in patron-title.inc

I found 3 occurrences where borrower is passed to the template:
members/pay.pl:        borrower => $borrower,
members/paycollect.pl:    borrower      => $borrower,
members/routing-lists.pl:    borrower          => $borrower,

$borrower comes from GetMember.

And from 1 members/moremember.pl (Koha::Patron)
So the change does not make sense.
But it's a regression of bug 12461, should be fixed elsewhere.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 33 Jonathan Druart 2017-05-04 14:09:27 UTC
Comment on attachment 62656 [details] [review]
Bug 18179: Forbid list context calls for Koha::Objects->find

See bug 18539 for this patch
Comment 34 Katrin Fischer 2017-05-13 12:04:50 UTC
(In reply to Jonathan Druart from comment #10)
> @RMaints, I would not backport the first patch, but the second one is safe
> ("Update existing calls").

Tried to backport, but there is a conflict in pay.pl. Can you take a look please?
Comment 35 Jonathan Druart 2017-05-29 18:02:50 UTC
(In reply to Jonathan Druart from comment #31)
> Created attachment 63108 [details] [review] [review]
> Bug 18179: Update 1 occurrence in moremember.pl

This patch is not longer needed, see bug 18647.
Comment 36 Jonathan Druart 2017-05-29 18:08:51 UTC
(In reply to Jonathan Druart from comment #32)
> Created attachment 63109 [details] [review] [review]
> Bug 18179: Use borrower.category_type in patron-title.inc

Same as previous comment
Comment 37 Jonathan Druart 2017-05-29 18:10:04 UTC
Created attachment 63832 [details] [review]
[16.11.x] Bug 18179: Update existing calls

This patch updates the existing occurrences of ->find called in a list
context.
There are certainly others that are not easy to catch with git grep.
Test plan:
Confirm that the 4 modified scripts still works as expected.

We need this one ASAP in master to make sure we will not get other
side-effects of this kind and to catch possible uncaught occurrences
before the release.

Tested scripts changed by this patch, they work as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 38 Jonathan Druart 2017-05-29 18:10:30 UTC
Only the first patch is needed for 16.11.x
Comment 39 Katrin Fischer 2017-06-05 13:50:10 UTC
Sorry, first patch listed here and first patch pushed do both not apply? 
It also looks like a part of the patches has been reverted from master and the status here is "Passed QA" - confusing!
Comment 40 Jonathan Druart 2017-06-05 14:56:20 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 41 Fridolin SOMERS 2017-06-09 10:47:46 UTC
Pushed to 17.05.x, will be in v17.05.01
Comment 42 Katrin Fischer 2017-06-11 19:38:44 UTC
This patch has been pushed to 16.11.x and will be in 16.11.09.

Picked the 16.11.x patch from here.
Comment 43 Marcel de Rooy 2017-06-26 08:05:27 UTC
Template process failed: undef error - Cannot use "->find" in list context at /usr/share/koha/masterclone/Koha/Club.pm line 51.

You will find a few with git grep "return.*->find"