Bug 15759 - Allow Koha::Object derived objects to be used as hashrefs
Summary: Allow Koha::Object derived objects to be used as hashrefs
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-08 15:53 UTC by Kyle M Hall
Modified: 2017-06-14 22:11 UTC (History)
3 users (show)

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


Attachments
Bug 15759 - Allow Koha::Object derived objects to be used as hashrefs (5.05 KB, patch)
2016-02-08 15:54 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2016-02-08 15:53:02 UTC
On occasion, bugs in Koha have been revealed to be a mis-use of Koha::Object. This seems to be due to our long standing use of hashrefs for data in Koha.

For instance, a developer may accidentally write:

my $borrowernumber = $borrower->{borrowernumber};

instead of

my $borrowernumber = $borrower->borrowernumber;

Since our object is a blessed hashref, this does not cause an error. Instead, we just access that key of the hashref, which is almost certainly undefined.

With some extra code in Koha/Object.pm, we can allow hashref-like use of our Objects, which will not only prevent these errors, but allow drop-in use of Koha::Objects where we currently use hashrefs!
Comment 1 Kyle M Hall 2016-02-08 15:54:35 UTC
Created attachment 47740 [details] [review]
Bug 15759 - Allow Koha::Object derived objects to be used as hashrefs

On occasion, bugs in Koha have been revealed to be a mis-use of
Koha::Object. This seems to be due to our long standing use of hashrefs
for data in Koha.

For instance, a developer may accidentally write:

my $borrowernumber = $borrower->{borrowernumber};

instead of

my $borrowernumber = $borrower->borrowernumber;

Since our object is a blessed hashref, this does not cause an error.
Instead, we just access that key of the hashref, which is almost
certainly undefined.

With some extra code in Koha/Object.pm, we can allow hashref-like use of
our Objects, which will not only prevent these errors, but allow drop-in
use of Koha::Objects where we currently use hashrefs!

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Borrower.t
Comment 2 Jonathan Druart 2016-06-27 12:03:45 UTC
This is an interesting approach, but I am not sure it will be good in the long term.
At the moment, when we see ->method we know we are manipulating an object, and with ->{key} that we are manipulating a hashref.
With this patch, we will never know easily.
It will permit an easier and quicker switch to Koha::Objects in several areas, but it will hide the job to be done.

We should at least warn/carp or use Koha::Log (debug) to know the places it is treated as a hashref.
My thinking is still that that can be achieve (less easily but less magic) using ->unblessed.
Comment 3 Kyle M Hall 2016-06-27 13:04:14 UTC
(In reply to Jonathan Druart from comment #2)

Yes, I do agree with your points. I'm on the fence post about this. I think there is a possibility of confusion, but I think the ability of being able to drop objects into legacy code may outweigh those problems in the short term. Long term we should definitely work to using our objects properly. Unblessed is very useful, but as soon as you unbless an object it loses all its powers. If we were to add this feature I think it would be safest to only use it at the script level, and not in our modules. It would provide incredible benefits in some of our scripts where switching the code to objects is very non-trivial due to all the processing, munching and additions to hashrefs that take place.

I'll bring it up at the next dev meeting so we can get a general consensus as to if this is a worthwhile feature to pursue. I'm fine with the decision going either way.
Comment 4 Alex Sassmannshausen 2016-07-07 11:30:31 UTC
(In reply to Kyle M Hall from comment #1)
> Created attachment 47740 [details] [review] [review]
> Bug 15759 - Allow Koha::Object derived objects to be used as hashrefs
> 
> On occasion, bugs in Koha have been revealed to be a mis-use of
> Koha::Object. This seems to be due to our long standing use of hashrefs
> for data in Koha.
> 
> For instance, a developer may accidentally write:
> 
> my $borrowernumber = $borrower->{borrowernumber};
> 
> instead of
> 
> my $borrowernumber = $borrower->borrowernumber;
> 
> Since our object is a blessed hashref, this does not cause an error.
> Instead, we just access that key of the hashref, which is almost
> certainly undefined.

I can see the problem here, but I am not sure about the proposed
solution.  I think the above should cause an error when it happens,
rather than Koha trying to hide the error.

> With some extra code in Koha/Object.pm, we can allow hashref-like use of
> our Objects, which will not only prevent these errors, but allow drop-in
> use of Koha::Objects where we currently use hashrefs!

To aid code legibility, it makes sense IMHO, to be clear about what we
are using.  The proposed change, while convenient, would obscure.

In terms of the conversion to Koha::Objects, I think it is better to
continue trudging through refactoring as we go.  It's work that needs
to be done anyway, and this is a good opportunity!

Alex