Page MenuHomePhabricator

Use a consistent getter for data access in VectorTemplate
Closed, ResolvedPublic

Description

VectorTemplate currently uses the following methods to access the data object used for populating the HTML:

1 $this->data['data’] - plain arrays

The following are inherited from QuickTemplate.php:
2 $this->html( 'data' )

 function html( $str ) {
    echo $this->data[$str];
}

3 $this->text( 'data' )

 function text( $str ) {
    echo htmlspecialchars( $this->data[$str] );
}

4 $this->get( 'data' )

 public function get( $name, $default = null ) {
    return $this->data[$name] ?? $default;
}

Out of these 4 methods, get() is the most valuable as it provides a default value (which is converted to an empty string in Mustache). #1 should be avoided, #2 is the same as number 1 and #3 is unnecessary because Mustache templates already sanitize the output between double curly braces. We should consolidate data access in VectorTemplate to convert the current methods to get().

Event Timeline

I would go even further, and I would make:

  • $data property private in the QuickTemplate.
  • add __get and __set methods, that check if someone tries to access $data variable. If yes, then do wfDeprecated(), and the allow it so it's backwards compatible.

In this way - we make the $data property not visible to other projects, but the existing skins/extensions will still work. After some time ( maybe 1.37 ) I would drop __get and __set and leave $data as private.
Sadly, this change will require us to update code for all skins that are used on Wikimedia foundation projects (CologneBlue, Minerva, Modern, MonoBook, Timeless, Vector) so it doesn't flood logs with deprecation notices.

Can't we just make html and text check the value exists (possibly giving them a default value that defaults to empty string instead of null? Is there any downside to making that check unconditionally? If so get could be deprecated for text which seems much more meaningful.

@Jdlrobson - yeah, we could add a default value of empty string. The only concern I have is that if someone makes a typo and tries to text( 'unknown' ) it would still work but with empty string - this can cause problems when debugging stuff. Maybe we could do sth like that"

public function text( $str, $defaultValue = null ) { 
  if ( !array_key_exists( $str, $this->data ) && $defaultValue === null ) {
     wfDebug( "Trying to access non-existing \"$str\" property from template data without defined default value");
  }
  return htmlspecialchars( $this->data[ $str ] ?? (string)$defaultText );
}

It's pretty easy to do a typo and it's pretty difficult to find why something doesn't render correctly.

Change 575615 had a related patch set uploaded (by Polishdeveloper; owner: Polishdeveloper):
[mediawiki/skins/Vector@master] Instead of accessing $data property use set/get/html methods

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

Change 575615 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Instead of accessing $data property use set/get/html methods

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

Jdlrobson added a subscriber: pmiazga.

This is done by our new volunteer @pmiazga - do we want to post-estimate or can I move this to sign off?

@Jdlrobson - pmiazga was the account I was using earlier, now it's @polishdeveloper :). Also, I'll create another task for other skins plus changes required in mwcore ( adding deprecation for $data property in QuickTemplate ).