Page MenuHomePhabricator

Refactor ConverterRule not to use public variables from LanguageConverter and remove mutual dependency between them
Open, HighPublic2 Estimated Story Points

Description

Motivation

A couple of variables has left unchanged (public) after T243461:

public $mFlags; // Used in ConverterRule
public $mDescCodeSep = ':'; // Used in ConverterRule, descendants
public $mDescVarSep = ';'; // Used in ConverterRule, descendants
public $mVariantNames; // Used in ConverterRule
public $mVariants; // Used in ConverterRule

At the moment ConverterRule uses them.

$validFlags = $this->mConverter->mFlags;

In order to make them non-public, ConverterRule should be refactored to get the required data, not object.

Expected result

  1. public $mConverter; should be removed from ConverterRule.
  2. All required data should be injected through constructor like below:
public function __construct( 
    $text, 
    $flags, 
    $convTable,
    ...
 ) {
    $this->mText = $text;
    $this->flags = $flags;
    ...
}
  1. Visibility for all variables should be decreased to private as there's no reason to use them outside ConverterRule itself.
public $mRuleDisplay = ''
...etc.

->

private $mRuleDisplay = ''
...etc.
  1. The class ConverterRule should be marked as @internal as this is internal Implementation and no reason to use it outside the LanguageConverter.
  2. LanguageConverter should be changed accordingly. Public variables should be changed to private.
public $mFlags;
...etc.

->

private $mFlags;
...etc.
  1. Respective changes should be added to RELEASE-NOTES-1.35

Event Timeline

Restricted Application removed a project: Patch-For-Review. · View Herald TranscriptJan 29 2020, 3:26 PM
Peter.ovchyn renamed this task from Refactor ConverterRule not to get public variables from LanguageConverter and remove mutual dependence between them to Refactor ConverterRule not to use public variables from LanguageConverter and remove mutual dependency between them.Jan 29 2020, 3:27 PM
daniel updated the task description. (Show Details)Feb 4 2020, 3:59 PM
Peter.ovchyn updated the task description. (Show Details)Feb 4 2020, 9:18 PM
Peter.ovchyn updated the task description. (Show Details)Feb 4 2020, 9:41 PM
Peter.ovchyn removed Peter.ovchyn as the assignee of this task.Feb 12 2020, 10:04 AM
Helga_sf set the point value for this task to 2.Apr 30 2020, 3:01 PM
Art-Baltai added a comment.EditedMay 16 2020, 1:28 AM

Expected Point:

  • public $mConverter; should be removed from ConverterRule.

not optimal because ConverterRule calls to $mConverter methods with dynamic arguments

mConverter->validateVariant( $v );
public function parse( $variant = null ) { ... mConverter->autoConvert( $this->mRules, $variant );
private function getRuleConvertedStr( $variant ) {  ...  mConverter->getVariantFallbacks( $variant ) );

and similar

Change 596798 had a related patch set uploaded (by Art-Baltai; owner: Art-Baltai):
[mediawiki/core@master] ConverterRule and LanguageConverter properties: make private or protected

https://gerrit.wikimedia.org/r/596798

not optimal because ConverterRule calls to $mConverter methods with dynamic arguments

Indeed! I missed that fact. Maybe it just worth hiding Converter's properties as much as possible as they're visible for all clients, not only Core.

Helga_sf triaged this task as High priority.May 25 2020, 11:48 AM
Helga_sf added a subscriber: Art-Baltai.
Helga_sf removed a subscriber: Art-Baltai.