Problem/Motivation

The ParserService is used by drupal_rest (DrupalRest) connector plugin to discover translatable strings from a release archive.

The implementation is currently messy, it will be hard to maintain and hard to verify that it actually works and properly covers different cases. It will also make it hard to reuse parts of the functionality elsewhere, e.g. for the local files plugin.

We are already creating tests in #3577854: Add tests for connector plugins.
And I already discovered a bug in #3582077: Reset global state between parse handler runs. which is exactly a consequence of one of the design problems (static variables).

Relevant flaws in the architecture:

  • Mixing of concerns in the same class:
    • Download and unpack release archive.
    • Discover translatable strings with potx.
    • Write entities for strings, lines and errors.
    • -> As a consequence, the discovery cannot be invoked without side effects (writing of entities)
  • Hidden global state side effects due to static variables.
    -> causes bugs if we forget to reset.
    -> hard to follow flow of information.
  • Mutation of object properties, instead of using parameters and return values.

Steps to reproduce

Proposed resolution

First we provide the tests in #3577854: Add tests for connector plugins.

Then we can refactor:

  • Split concerns into separate classes.
  • Introduce value objects.
  • Make most classes immutable.
    (Builder pattern can be used)
  • Get rid of static variables and global state outside of potx.
  • Review exception handling.

I already have all of this as preview branch...

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork l10n_server-3582331

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 !120

donquixote’s picture

donquixote commented
Status: Active » Needs review

Review time!
Looking for general feedback and opinions, but also we could look at ways to split this up.
(Although I already did split out some parts)

gábor hojtsy made their first commit to this issue’s fork.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Reviewed and rebased the MR onto current 3.0.x, so the local file system connector can be built on it (the local files plugin the summary names as the motivation).

Rebase. Seven of the thirty commits conflicted with what landed since June: the config schema store (#3621250: Config schema parsed from one release is not kept for parsing other releases) injects PotxSchemaStore into the parser and replaces potx_local_init(), the scanner autowiring (#3563318: Improvements to scan handling), and the release core and branch helpers (#3621249: Release core, branch and version derived from the label instead of the version number) that made L10nHelper::releaseSetBranch() redundant. Resolved by keeping the schema store hook in the new PotxTreeParser and dropping the helper.

Follow-up commit on top:

  • The two tests that still used L10nHelper go through the new tree parser and entity writer, and the reset release test compares integer ids now that id() returns int.
  • PotxTreeParser moved from the rest connector into l10n_server next to the entity writer: it is the shared part, the rest connector only adds download and unpack, and the local files connector will use it as is.
  • ReleaseInventoryEntityWriter used PHPUnit\Framework\Assert at runtime, replaced with assert().

Review. The split into a tree loader, a potx tree parser producing an inventory value object, and an entity writer is the great, and clearing the potx global state around each run is a nice improvement. Two things noticed for later, not changed here: the unpacked tree and the downloaded archive are never deleted, so every parsed release leaves its tree in the temporary directory (the old code had the same gap), and the tar call is built with escapeshellcmd() on the whole command, which does not protect a path with a space. Both are addressed in the local files connector issue that builds on this. The integer id interfaces on all entities are unrelated to the parser but harmless.

LLM was used to find, diagnose explain and fix this issue. With human review.

  • gábor hojtsy committed 20e8d26f on 3.0.x 
    Merge branch '3582331-refactor-parserservice-potx' into '3.0.x'

Resolve...

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Fixed

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.

Status: Fixed » Closed (fixed)

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