Problem/Motivation

The static variables in L10nHelper are not reset.
This leads to wrong behavior on repeated calls. Strings are created with the wrong file entity referenced.

Steps to reproduce

The tests in #3577854: Add tests for connector plugins reveal the bad behavior, and they will be fixed here.

Proposed resolution

Merge this first: #3577854: Add tests for connector plugins!

Implement a reset mechanism.

Normally we should go a bit further and move static variables from inside methods to class level.
But I am planning some more refactoring, so I want to keep this bug fix minimal.

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork l10n_server-3582077

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

donquixote created an issue. See original summary.

donquixote opened merge request !119

donquixote’s picture

donquixote commented
Status: Active » Needs review
donquixote’s picture

donquixote commented
Title: Reset static variables in L10nHelper » Reset global state between parse handler runs.

fmb made their first commit to this issue’s fork.

fmb’s picture

fmb commented

Normally we should go a bit further and move static variables from inside methods to class level.
But I am planning some more refactoring, so I want to keep this bug fix minimal.

So I take it this is a temporary solution, right? I understand how and why you are resetting global state, but honestly this is a blatant example why we should limit global state in the first place...

donquixote’s picture

donquixote commented

So I take it this is a temporary solution, right? I understand how and why you are resetting global state, but honestly this is a blatant example why we should limit global state in the first place...

Indeed it is meant to be temporary.
I have a big refactor in the pipeline, but I wanted to do these bug fixes first, so that we can separate refactoring from fixing, an have stable tests that cover these cases. I felt like the refactoring is too ambitious to include and delay all kinds of bug fixes.
(In its original meaning, refactoring does not include bug fixes or new features)

However, the refactoring only removes the global state in l10n_server package in the L10nHelper class.
It does not remove global state in potx module.
Gabor wants to be able to backport everything to potx for Drupal 7, this is why we don't see it change much.

pierregermain’s picture

pierregermain commented
Status: Needs review » Reviewed & tested by the community

  • fmb committed 56de588a on 3.0.x 
    Merge branch '3582077-reset-static-vars-in-L10nHelper' into '3.0.x'...

  • fmb committed a79739c0 on 3.0.x authored by donquixote 
    Issue #3582077: Test global state reset between runs in ParserService.

  • fmb committed 2ebdbbbf on 3.0.x authored by donquixote 
    Issue #3582077: Fix: Clear and then restore all relevant global state...
fmb’s picture

fmb commented
Status: Reviewed & tested by the community » Fixed

Alright, this is now merged, thanks.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

donquixote’s picture

donquixote commented
Related issues: +#3582331: Refactor ParserService potx string discovery

@fmb You asked if we could remove the global state.
Here we go :) #3582331: Refactor ParserService potx string discovery

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.