Page MenuHomePhabricator

Decrease visibility of public variables in LanguageConverter class
Closed, ResolvedPublic

Description

public $mLangObj;

Actual:

    public $mFlags;
	public $mDescCodeSep = ':', $mDescVarSep = ';';
	public $mUcfirst = false;
	public $mConvRuleTitle = false;
	public $mURLVariant;
	public $mUserVariant;
	public $mHeaderVariant;
	public $mMaxDepth = 10;
	public $mVarSeparatorPattern;

Expected:

protected $mFlags;

Details

Event Timeline

Restricted Application removed a project: Patch-For-Review. · View Herald TranscriptJan 22 2020, 9:32 PM

This definitely seems like a good idea. The trick is making sure that nothing relies on these fields. We don't have a formal deprecation process for reducing visibility, but it's technically removing something from the public interface, so care should be taken.

Peter.ovchyn added a comment.EditedJan 24 2020, 10:56 AM

The trick is making sure that nothing relies on these fields. We don't have a formal deprecation process for reducing visibility, but it's technically removing something from the public interface, so care should be taken.

I see two attempts to normalize property deprecation:

  • DeprecationHelper
  • DeprecatedGlobal

And none is really used widely. Why?

Looks like DeprecationHelper is more or less good for that.
What if we use it here?

Moreover, I'd improve it a bit in order to provide a better strategy of deprecation as separate ticket:
I see the following cases:

  1. We want to deprecate direct set only. - Not sure this a real case.
  2. We want to deprecate both 'set ' and 'get', but leave it as property in the class. - Good for decreasing visibility etc..
  3. We want to deprecate both 'set ' and 'get' and temporarily bind the property to some callback. The deprecation of Language::$mConverter is a good example:
public function __get( string $name ) {
		if ( $name == "mConverter" ) {
			wfDeprecated( 'mConverter', '1.35' );
			return $this->getConverter();
		}
		throw new RuntimeException( "Cannot get '$name' property." );
	}

Ah, good find! I was actually looking for DeprecationHelper the other day, and didn't remember the name. Yes, it's a good fit for this used case. It's not used much because on the one hand, few people know about it. On the other hand, it's never used for long - any use should be removed after one release.

I don't think use case (1) exist. When we don't ant direct writes, we don't want direct reads either.

Change 568505 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/core@master] languages: Decrease visibility of public variables in LanguageConverter class

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

A couple of variables has left unchanged:

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

In order to make them non-public, ConverterRule should be refactored.
I created a task on that.

Change 568505 merged by jenkins-bot:
[mediawiki/core@master] languages: Decrease visibility of public variables in LanguageConverter class

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

Peter.ovchyn closed this task as Resolved.Feb 10 2020, 7:47 PM