Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Zonemaster::Engine::Translator’s instance() method #1347

Merged

Conversation

marc-vanderwal
Copy link
Contributor

@marc-vanderwal marc-vanderwal commented May 15, 2024

Purpose

This PR fixes a bug in Zonemaster::Engine::Translator that causes problems in Zonemaster::Backend::Translator (in Backend, not Engine), which inherits from Zonemaster::Engine::Translator.

Context

While adding unit tests in Zonemaster::Backend for its Translator module, I noticed that the instance() method in Zonemaster::Engine::Translator always returned undef instead of a Translator object.

Changes

  • Fix a bug in the instance() method which prevented it from working as documented.
  • Fix small mistakes in the module’s documentation.
  • Change unit test so that it calls instance() instead of new(), to exercise the code.

How to test this PR

Run the test suite in Zonemaster::Engine. Expect all unit tests to pass.

A programming error in Zonemaster::Engine::Translator’s instance()
method caused the global variable $instance to be immediately undefined
right after the initialize() function set it to an instance of a
Translator object, if that singleton object hadn’t been previously
instanciated.

The documentation for Zonemaster::Engine::Translator’s initialize()
method was also incorrect: this function takes a raw list of arguments,
not a hash reference.

Do not use the new() method in the corresponding unit tests anymore,
because that method is deprecated.
@marc-vanderwal marc-vanderwal added T-Bug Type: Bug in software or error in test case description V-Patch Versioning: The change gives an update of patch in version. labels May 15, 2024
@marc-vanderwal marc-vanderwal added this to the v2024.1 milestone May 15, 2024
@marc-vanderwal
Copy link
Contributor Author

I’m moving one commit from this PR into its neighbor #1346, because it’s more relevant there than here. The idea is to make #1346 address the most urgent issues in Zonemaster::Engine::Translator and this one the more latent problems.

The documentation for Zonemaster::Engine::Translator still referred to
Moose in one place.
@marc-vanderwal
Copy link
Contributor Author

I’m keeping this PR as a draft for now, because I’m considering also deprecating the new() method altogether (as is stated in the documentation). A proper singleton class ought to have a private constructor and only expose the instance() function. If so, I’ll need to remember to change the V-Patch tag to V-Major.

@marc-vanderwal marc-vanderwal changed the title Fix multiple regressions in Engine’s and Backend’s Translator modules Fix Zonemaster::Engine::Translator’s instance() method May 16, 2024
@marc-vanderwal
Copy link
Contributor Author

I’m keeping this PR as a draft for now, because I’m considering also deprecating the new() method altogether (as is stated in the documentation). A proper singleton class ought to have a private constructor and only expose the instance() function. If so, I’ll need to remember to change the V-Patch tag to V-Major.

That’s better done in another PR.

@marc-vanderwal marc-vanderwal marked this pull request as ready for review May 16, 2024 07:30
@matsduf matsduf added the P-High Priority: Issue to be solved before other label May 22, 2024
@marc-vanderwal marc-vanderwal merged commit 8bdc426 into zonemaster:develop Jun 3, 2024
3 checks passed
marc-vanderwal added a commit to marc-vanderwal/zonemaster-backend that referenced this pull request Jun 3, 2024
Pull request zonemaster-engine#1347
(zonemaster/zonemaster-engine#1347) has been
merged, which fixes Zonemaster::Backend::Translator->instance(). So we
can remove one use of the deprecated
Zonemaster::Backend::Translator->new() in the unit tests.
@marc-vanderwal marc-vanderwal deleted the bugfix/misc-translator-bugs branch June 12, 2024 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-High Priority: Issue to be solved before other T-Bug Type: Bug in software or error in test case description V-Patch Versioning: The change gives an update of patch in version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants