Bug 9006 - autoload in C4::Context is a bad idea
Summary: autoload in C4::Context is a bad idea
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Jonathan Druart
QA Contact:
URL:
Keywords:
Depends on: 14428
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-01 19:09 UTC by Jared Camins-Esakov
Modified: 2016-12-05 21:22 UTC (History)
4 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 9006: Remove AUTOLOAD in C4::Context (9.38 KB, patch)
2015-04-01 12:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9006: Remove AUTOLOAD in C4::Context (4.52 KB, patch)
2015-06-19 08:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9006: Remove AUTOLOAD in C4::Context (4.98 KB, patch)
2015-07-10 13:04 UTC, Mark Tompsett
Details | Diff | Splinter Review
[PASSED QA] Bug 9006: Remove AUTOLOAD in C4::Context (5.06 KB, patch)
2015-07-10 14:42 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 Jared Camins-Esakov 2012-11-01 19:09:13 UTC
The comment in C4/Context.pm pretty much sums it up:
# AUTOLOAD
# This implements C4::Config->foo, and simply returns
# C4::Context->config("foo"), as described in the documentation for
# &config, above.

# FIXME - Perhaps this should be extended to check &config first, and
# then &preference if that fails. OTOH, AUTOLOAD could lead to crappy
# code, so it'd probably be best to delete it altogether so as not to
# encourage people to use it.
Comment 1 Jonathan Druart 2015-04-01 12:01:34 UTC Comment hidden (obsolete)
Comment 2 Mark Tompsett 2015-05-04 04:11:00 UTC
$ grep "^sub " C4/Context.pm | cut -f2 -d' ' > ~/subs
$ vi ~/subs
:%s/^/C4::Context->/g
:wq
$ git grep 'C4::Context\->' | grep -v -f ~/subs

Yes, intranetdir, but also:
1 perldoc line IsSuperlibrarian typo
something ugly in C4/Input.pm
typo in 3.07.00.030 updatedatabase.pl upgrade

Perhaps those deserve a bug report?
Comment 3 Mark Tompsett 2015-05-04 04:28:17 UTC
Koha::FrameworkPlugin needs tweaking, and is the reason a lot of the other stuff doesn't apply. Please rebase, amend, test, and reattach for signoff. :)
Comment 4 Jonathan Druart 2015-06-19 08:39:10 UTC Comment hidden (obsolete)
Comment 5 Mark Tompsett 2015-06-19 16:44:55 UTC
I re-ran my commands in comment #2

$ grep "^sub " C4/Context.pm | cut -f2 -d' ' > ~/subs
$ vi ~/subs
:%s/^/C4::Context->/g
:wq
$ git grep 'C4::Context\->' | grep -v -f ~/subs

Yes, intranetdir, but also:
1 perldoc line IsSuperlibrarian typo (see also bug 14425)
something ugly in C4/Input.pm (patch to come on this bug)
typo in 3.07.00.030 updatedatabase.pl upgrade (see also bug 14422)

But I discovered that bug 13876 introduced C4::Context->KOHAVERSION.
I think it should 'use Koha;' and then be 'Koha::version';
Also, this likely should be a patch on this bug.
Comment 6 Mark Tompsett 2015-06-19 22:45:24 UTC
(In reply to M. Tompsett from comment #5)
> something ugly in C4/Input.pm (patch to come on 14428, though perhaps C4::Input should be removed)
...
> But I discovered that bug 13876 introduced C4::Context->KOHAVERSION.
> I think it should 'use Koha;' and then be 'Koha::version';
This is on bug 14427.
Comment 7 Mark Tompsett 2015-07-10 05:28:44 UTC
Setting this to depends on 14428, because 14428 removes the one non-intranetdir autoload by removing C4::Input (good bye, crud!).
Comment 8 Jonathan Druart 2015-07-10 07:15:22 UTC
(In reply to M. Tompsett from comment #7)
> Setting this to depends on 14428, because 14428 removes the one
> non-intranetdir autoload by removing C4::Input (good bye, crud!).

This cans still be signed off.
Comment 9 Mark Tompsett 2015-07-10 13:04:38 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2015-07-10 14:42:54 UTC
Created attachment 40924 [details] [review]
[PASSED QA] Bug 9006: Remove AUTOLOAD in C4::Context

Happily this was only used for intranetdir.
It's time to remove it and replace existing calls.

I used the following commands to catch calls to C4::Context:
git grep 'C4::Context\->' | grep -v 'C4::Context->preference' | grep -v
'C4::Context->config' | grep -v  'C4::Context->userenv' | grep -v
'C4::Context->IsSuperLibrarian' | grep -v 'C4::Context->dbh' | grep -v
'C4::Context->set_preference' | grep -v '_syspref_cache' | grep -v
_userenv | grep -v 'C4::Context->interface' | grep -v
'C4::Context->Zconn' | grep -v 'C4::Context->queryparser' | grep -v
'C4::Context->tz' | grep -v 'C4::Context->boolean_preference' | grep -v
'C4::Context->memcached'

NOTE: I applied 14428, and then did what I suggested in comment #2.
      Only intranetdir references appeared.

      I applied this patch, and repeated.
      Nothing appeared. This means the autoload references are
      properly removed.

      koha qa test tools complained about whitespace, I just fixed
      those. Though, we may wish to perltidy
      auth_fields_substructure.pl on another bug.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Tomás Cohen Arazi 2015-07-20 13:29:03 UTC
Patch pushed to master.

Thanks Jonathan!