Bug 35133 - Accessors defined in AUTOLOAD does not work if called with SUPER
Summary: Accessors defined in AUTOLOAD does not work if called with SUPER
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Gustafsson
QA Contact: Emily Lamancusa
URL:
Keywords:
Depends on:
Blocks: 32476
  Show dependency treegraph
 
Reported: 2023-10-23 15:23 UTC by David Gustafsson
Modified: 2024-01-18 21:20 UTC (History)
2 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 35133: Don't lazily define accessor methods when invoked through SUPER (1.07 KB, patch)
2023-10-23 15:51 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 35133: Don't lazily define accessor methods when invoked through SUPER (1.36 KB, patch)
2023-10-25 12:21 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 35133: Don't lazily define accessor methods when invoked through SUPER (1.42 KB, patch)
2024-01-10 17:07 UTC, Matthias Le Gac
Details | Diff | Splinter Review
Bug 35133: Don't lazily define accessor methods when invoked through SUPER (1.48 KB, patch)
2024-01-10 17:09 UTC, Phan Tung Bui
Details | Diff | Splinter Review
Bug 35133: Don't lazily define accessor methods when invoked through SUPER (1.55 KB, patch)
2024-01-18 20:58 UTC, Emily Lamancusa
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Gustafsson 2023-10-23 15:23:21 UTC

    
Comment 1 David Gustafsson 2023-10-23 15:38:02 UTC
If overriding a accessor method in a child class of Koha::Object, and calling parent by invoking $self->SUPER->accessor_name, $AUTOLOAD in the AUTOLOAD method of KOHA::Object will be assigned the value  Koha::ChildClass::SUPER::accessor_name instead of Koha::ChildClass::accessor_name. The next time $self->SUPER->accessor_name is called it will not be picked up, and AUTOLOAD will run again with a SubroutineKoha::ChildClass::SUPER::accessor_name  redefined" warning, so obviously definition of lazy accessors does not work for those cases.

I tried digging around to see if there is some way of defining autoloaded accessors that handles this case, but unfortunately did not find any relevant information on this. The only, probably safe, workaround is to not insert an entry into the dispatch table when $AUTOLOAD contains '::SUPER::', so until we have a better solution I think that probably is the way to go.
Comment 2 David Gustafsson 2023-10-23 15:51:51 UTC
Created attachment 157678 [details] [review]
Bug 35133: Don't lazily define accessor methods when invoked through SUPER
Comment 3 David Gustafsson 2023-10-23 16:01:46 UTC
I will attach a test case tomorrow, it's a shame it seems so hard to properly implement lazy accessors without resorting to this ugly fix. If someone can come up with some other suggestions that would be great.
Comment 4 David Cook 2023-10-24 00:23:12 UTC
It would be a good idea to link to the previous dev via "Depends on" or at least "See Also"
Comment 5 David Gustafsson 2023-10-25 12:21:37 UTC
Created attachment 157793 [details] [review]
Bug 35133: Don't lazily define accessor methods when invoked through SUPER

To test:
  1) Apply Bug 32476
  2) Run tests in t/db_dependent/Patrons.t
  3) This should produce multiple "Subroutine Koha::Patron::SUPER::dateexpiry redefined at ..." warnings
  4) Apply patch
  5) Run tests in t/db_dependent/Patrons.t again
  6) The warnings in 3) should now no longer appear
Comment 6 Matthias Le Gac 2024-01-10 17:07:23 UTC
Created attachment 160777 [details] [review]
Bug 35133: Don't lazily define accessor methods when invoked through SUPER

To test:
  1) Apply Bug 32476
  2) Run tests in t/db_dependent/Patrons.t
  3) This should produce multiple "Subroutine Koha::Patron::SUPER::dateexpiry redefined at ..." warnings
  4) Apply patch
  5) Run tests in t/db_dependent/Patrons.t again
  6) The warnings in 3) should now no longer appear

Signed-off-by: matthias le gac <matthias@babel.inlibro.net>
Comment 7 Phan Tung Bui 2024-01-10 17:09:58 UTC
Created attachment 160778 [details] [review]
Bug 35133: Don't lazily define accessor methods when invoked through SUPER

To test:
  1) Apply Bug 32476
  2) Run tests in t/db_dependent/Patrons.t
  3) This should produce multiple "Subroutine Koha::Patron::SUPER::dateexpiry redefined at ..." warnings
  4) Apply patch
  5) Run tests in t/db_dependent/Patrons.t again
  6) The warnings in 3) should now no longer appear

Signed-off-by: matthias le gac <matthias@babel.inlibro.net>
Signed-off-by: Phan Tung Bui <phan-tung.bui@inlibro.com>





Phan Tung Bui
Comment 8 Emily Lamancusa 2024-01-18 20:58:10 UTC
Created attachment 161152 [details] [review]
Bug 35133: Don't lazily define accessor methods when invoked through SUPER

To test:
  1) Apply Bug 32476
  2) Run tests in t/db_dependent/Patrons.t
  3) This should produce multiple "Subroutine Koha::Patron::SUPER::dateexpiry redefined at ..." warnings
  4) Apply patch
  5) Run tests in t/db_dependent/Patrons.t again
  6) The warnings in 3) should now no longer appear

Signed-off-by: matthias le gac <matthias@babel.inlibro.net>
Signed-off-by: Phan Tung Bui <phan-tung.bui@inlibro.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 9 Emily Lamancusa 2024-01-18 21:20:39 UTC
Looks like this has been a bug in Perl since version 5.18 - https://www.perlmonks.org/?node_id=1171033

It makes sense to me to avoid adding the method reference to the symbol table when handling a SUPER call, since the reference will not work and the next call will fall back to AUTOLOAD anyway. This shouldn't have any side effects, and it passes the QA checks. Thanks for including a clear explanation in the comments, too!