Page MenuHomePhabricator

HTMLFormField and subclasses use static::class to make css class names
Closed, ResolvedPublic

Description

$fieldType = static::class;
...
			$html .= Html::rawElement( 'tr',
				$rowAttributes + [
					'class' => "mw-htmlform-field-$fieldType {$this->mClass} $errorClass $rowClasses"
				],
				$field );

So this is fine when a class is not namespaced, but if you try and namespace the class (as I'm trying to do in BetaFeatures), this changes the class name applied on the element.

The expected <tr class="mw-htmlform-field-HTMLFeatureField"> becomes <tr class="mw-htmlform-field-MediaWiki\Extension\BetaFeatures\HTMLFeatureField">

While this is possibly fine, it feels unexpected. And I wouldn't have noticed without the tests inside BetaFeatures, until maybe someones CSS gets broken somewhere in production.

It's obivously easy enough to "workaround" this by adding a cssclass entry for the old name, and then update the tests (if applicable) to match.

It does lead me to wonder if I (or anyone else) has "broken" things in similar situations when namespacing extensions. Other possible examples could be HTMLFormRadioRangeColumnLabels in MediaWiki-extensions-SecurePoll... Where a cssclass replacement wasn't added.

It would be possible to add a constructor (if one doesn't exist) already into the subclass, and then inject/add a cssclass entry, such as the below... Which seems slightly preferable to adding a cssclass at every usage point.

	public function __construct( $params ) {
		if ( isset( $params['cssclass'] ) ) {
			$params['cssclass'] .= ' mw-htmlform-field-HTMLFormRadioRangeColumnLabels';
		} else {
			$params['cssclass'] = 'mw-htmlform-field-HTMLFormRadioRangeColumnLabels';
		}
		parent::__construct( $params );
	}

Or the other option is doing something (reflection based?) to look if the class is namespaced, and if it is, add an extra class... Or unconditionally just do str_replace( __NAMESPACE__ . '\\', '', get_class() ) and drop the namespacing entirely...

Event Timeline

I'd do the str_replace and not think about it too much.

Change 672533 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/core@master] HTMLFormField: Use non namespaced class name rather than static::class

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

Change 672533 merged by jenkins-bot:
[mediawiki/core@master] HTMLFormField: Use non namespaced class name rather than static::class

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

Change 674376 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/core@REL1_35] HTMLFormField: Use non namespaced class name rather than static::class

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

Change 674377 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/core@REL1_31] HTMLFormField: Use non namespaced class name rather than static::class

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

matmarex assigned this task to Reedy.

Change 674377 merged by jenkins-bot:
[mediawiki/core@REL1_31] HTMLFormField: Use non namespaced class name rather than static::class

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

Change 674376 merged by jenkins-bot:
[mediawiki/core@REL1_35] HTMLFormField: Use non namespaced class name rather than static::class

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