Bug 14066 - Koha code does not really use Readonly
Summary: Koha code does not really use Readonly
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 16588
  Show dependency treegraph
 
Reported: 2015-04-27 11:40 UTC by Jonathan Druart
Modified: 2016-07-22 08:08 UTC (History)
5 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:


Attachments
Bug 14066: Remove the Readonly[::XS] dependency (2.21 KB, patch)
2015-04-27 11:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14066: Correct and optimize (1.84 KB, patch)
2015-04-27 22:39 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14066: Correct and optimize (1.80 KB, patch)
2015-04-27 22:51 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14066: Remove the Readonly[::XS] dependency (2.26 KB, patch)
2015-04-27 22:52 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14066: Correct and optimize (1.80 KB, patch)
2015-04-27 22:52 UTC, Mark Tompsett
Details | Diff | Splinter Review
[Signed-off] Bug 14066: Remove the Readonly[::XS] dependency (2.41 KB, patch)
2015-04-28 04:46 UTC, Marc Véron
Details | Diff | Splinter Review
[Signed-off] Bug 14066: Remove the Readonly[::XS] dependency (2.41 KB, patch)
2015-04-28 04:59 UTC, Marc Véron
Details | Diff | Splinter Review
[Signed-off] Bug 14066: Correct and optimize (1.99 KB, patch)
2015-04-28 05:02 UTC, Marc Véron
Details | Diff | Splinter Review
[PASSED QA] Bug 14066: Remove the Readonly[::XS] dependency (2.46 KB, patch)
2015-05-03 20:06 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 14066: Correct and optimize (2.04 KB, patch)
2015-05-03 20:06 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14066: Removing more Readonly (2.29 KB, patch)
2015-05-03 20:16 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2015-04-27 11:40:31 UTC
The Readonly and Readonly::XS package are only used in Koha::Calendar.
IMO the added value of there packages does not justify their purpose.
I would suggest to remove these packages as Koha deps.
Comment 1 Jonathan Druart 2015-04-27 11:44:15 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2015-04-27 11:45:26 UTC
Colin, do you have any concerns about this patch?
Comment 3 Mark Tompsett 2015-04-27 15:09:13 UTC
Comment on attachment 38546 [details] [review]
Bug 14066: Remove the Readonly[::XS] dependency

Review of attachment 38546 [details] [review]:
-----------------------------------------------------------------

::: C4/Installer/PerlDependencies.pm
@@ -240,5 @@
> -    'Readonly' => {
> -        'usage'    => 'Core',
> -        'required' => '1',
> -        'min_ver'  => '1.03'
> -    },

I think we should keep Readonly. It is one of the better ways of defining constants, and would allow us to improve our perlcritic levels in the future. Currently, we only require perlcritic -5.

@@ -245,5 @@
> -    'Readonly::XS' => {
> -        'usage'    => 'Core',
> -        'required' => '0',
> -        'min_ver'  => '1.02'
> -    },

However, from the Readonly page on CPAN:
"I repeat, you do not need Readonly::XS if your environment has perl 5.8.x or higher."
Perhaps just removing Readonly::XS is the better solution. :)

::: Koha/Calendar.pm
@@ +126,4 @@
>  
>      if ( $unit eq 'hours' ) {
>          # Fixed for legacy support. Should be set as a branch parameter
> +        my $return_by_hour => 10;

Readonly's definition of a scalar value is confusing, such that when you changed it to a simple variable to accidentally left what looks like hashiness (=>), even though it probably should be plainly scalar (=).
Comment 4 Mark Tompsett 2015-04-27 15:36:23 UTC
Some interesting reading while researching making things read only.

"The interface for this module was inspired by Eric Roode's Readonly. The implementation is inspired by doing everything the opposite way Readonly does it."
(http://search.cpan.org/~leont/Const-Fast-0.014/lib/Const/Fast.pm)
Comment 5 Mark Tompsett 2015-04-27 15:50:14 UTC
Further reading.
http://neilb.org/reviews/constants.html
Comment 6 Mark Tompsett 2015-04-27 22:39:26 UTC Comment hidden (obsolete)
Comment 7 Mark Tompsett 2015-04-27 22:51:26 UTC Comment hidden (obsolete)
Comment 8 Mark Tompsett 2015-04-27 22:52:09 UTC Comment hidden (obsolete)
Comment 9 Mark Tompsett 2015-04-27 22:52:22 UTC Comment hidden (obsolete)
Comment 10 Mark Tompsett 2015-04-27 22:55:39 UTC
P.S. I did test around in Home->Tools->Calendar too, and attempting to look for sunday elsewhere in code.

P.P.S. It's beautiful to have a white Perl modules tab.
Comment 11 Marc Véron 2015-04-28 04:43:00 UTC
Testing with both patches, following test plan:

1) Apply both patches

OK

2) prove -v t/db_dependent/Calendar.t

?? t/db_dependent/Calendar.t
This file does not exist.
find -name Cal*.t
Result: ./t/Calendar.t
OK - error in test plan :-)

3) prove -v t/db_dependent/Holidays.t
OK

4) prove -v t/Calendar.t
OK

5) perlcritic -3 Koha/Calendar.pm
   -- the message corresponds to a comment in the code already
      and not this change.
OK, one message found: 
Package variable declared or used at line 60, column 1.  See pages 73,75 of PBP.  (Severity: 3)

6) run koha qa test tools
qa -c 2 [-v 2]
OK	Koha/Calendar.pm
OK	C4/Installer/PerlDependencies.pm

Will sign off...
Comment 12 Marc Véron 2015-04-28 04:46:31 UTC Comment hidden (obsolete)
Comment 13 Marc Véron 2015-04-28 04:53:25 UTC
Comment on attachment 38591 [details] [review]
[Signed-off] Bug 14066: Remove the Readonly[::XS] dependency

Mistaked (once more...), sorry
Comment 14 Marc Véron 2015-04-28 04:59:57 UTC Comment hidden (obsolete)
Comment 15 Marc Véron 2015-04-28 05:02:11 UTC Comment hidden (obsolete)
Comment 16 Katrin Fischer 2015-05-03 20:06:43 UTC
Created attachment 38787 [details] [review]
[PASSED QA] Bug 14066: Remove the Readonly[::XS] dependency

The Readonly and Readonly::XS package are only used in Koha::Calendar.
IMO the added value of there packages does not justify their purpose.

Test plan:
1/ Remove Readonly and Readonly::XS
2/ Open the calendar and confirm you don't get any error concerning missing
dependency

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 17 Katrin Fischer 2015-05-03 20:06:56 UTC
Created attachment 38788 [details] [review]
[PASSED QA] Bug 14066: Correct and optimize

As far as I can tell $sunday is never used. And it seemed to be
scoped to only _init.

The remaining hashiness was cleaned up for the $return_by_hour.

TEST PLAN
---------
1) Apply both patches
2) (Skip this one - file does not exist / MV) prove -v t/db_dependent/Calendar.t
3) prove -v t/db_dependent/Holidays.t
4) prove -v t/Calendar.t
5) perlcritic -3 Koha/Calendar.pm
   -- the message corresponds to a comment in the code already
      and not this change.
6) run koha qa test tools

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 18 Katrin Fischer 2015-05-03 20:08:30 UTC
Do we need a patch to remove from debian/control as well? 

I will attach a follow-up removing some more occurrences.
Comment 19 Katrin Fischer 2015-05-03 20:16:25 UTC
Created attachment 38789 [details] [review]
Bug 14066: Removing more Readonly
Comment 20 Tomás Cohen Arazi 2015-05-06 15:09:31 UTC
Patches pushed to master.

Thanks everyone!