Page MenuHomePhabricator

Improve file attribution for “Called deprecation function” logs
Open, Needs TriagePublic

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

Krinkle created this task.Feb 12 2020, 11:40 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 12 2020, 11:40 PM

(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.

Krinkle added a subscriber: daniel.EditedFeb 12 2020, 11:51 PM

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

Anomie added a subscriber: Anomie.Feb 14 2020, 9:36 PM

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.