Page MenuHomePhabricator

Improve file attribution for “Called deprecation function” logs
Closed, ResolvedPublic1 Estimated Story Points

Description

Looks like for some cases wfDeprecated() or a layer on top of it is not-so-helpfully attributing the caller as the same class where the deprecation itself came from. For example DefaultPreferenceFactory has a deprecation currently, but when it is triggered it says "Called from DefaultPreferenceFactory.php" instead of the actual caller above that.

Event Timeline

(Originally reported by @Nikerabbit during a TechCom meeting, please elaborate/correct if I misunderstood).

This is implemented by DeprecationHelper::newArgumentWithDeprecation which seems to be an unneccecary abstraction layer for a simple conditoinal that calls wfDeprecated and sets an default value.

Expected
	if ( !$languageConverter ) {
		wfDeprecated( __METHOD__ . ' without $languageConverter', '1.35' );
		$languageConverter = MediaWikiServices::getInstance()->getLanguageConverterFactory()
					->getLanguageConverter();
	}
	$this->languageConverter = $languageConverter;
Actual
	$this->languageConverter = DeprecationHelper::newArgumentWithDeprecation(
		__METHOD__,
		'languageConverter',
		'1.35',
		$languageConverter,
		function () {
			return MediaWikiServices::getInstance()->getLanguageConverterFactory()
				->getLanguageConverter();
		}
	);

This abstraction doesn't appear to add value in the form of re-used logic, separated concern, or simpler code. And in this case it also means that wfDeprecated() is called one layer too far away from the code triggering new DefaultPreferenceFactory and thus it doesn't report the file that uses it, but it reports DefaultPreferenceFactory.php as the caller.

Traced back to https://gerrit.wikimedia.org/r/569028 CC-ring reviewer @daniel to follow-up to ensure the trace offset is correct (or use the simpler form we use elsewhere).

I agree with fixing the wrong caller. But I totally disagree with the fact that this layer isn't useful. It has a couple advantages:

  1. It encapsulates all logic inside so that you shouldn't write if ( !$languageConverter ) {
  2. Standardize the deprecation of outdated signatures. So that it can be used everywhere.
  3. It can easily be matched by automation tools so that we won't forget to remove the next versions.
  4. We use wfDeprecation for different purposes:
  5. signature changes
  6. public variables deprecation (decreasing the visibility or moving variable to new class etc..)
  7. Function deprecations

...

Instead of getting back the original approach, I'd rather create a comprehensive list of all possible usage and create an explicit method for each of them.

Change 572223 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/core@master] deprecation: Improve file attribution for “Called deprecation function” logs

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

I think I agree with @Krinkle on this, turning things inside out with DeprecationHelper being passed a callback seems to have no advantage here over the more straightforward code calling wfDeprecated(). In my opinion, it actually makes the code more confusing.

Peter.ovchyn set the point value for this task to 1.Apr 14 2020, 9:43 AM
Peter.ovchyn triaged this task as Medium priority.

Change 572223 merged by jenkins-bot:
[mediawiki/core@master] deprecation: Remove DeprecationHelper::newArgumentWithDeprecation and change callers accordingly

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