Page MenuHomePhabricator

Provide basic FeatureManagement in Vector codebase
Closed, ResolvedPublic8 Estimated Story Points

Description

Task

In order to easily manage DesktopImprovements feature, provide a simple mechanism that allows engineers to:

  • easily register new features
  • allow admins to enable/disable feature by editing well defined configuration
  • allow user to opt-in / opt-out to Desktop Improvements project

Status (2020/04/28)

Vector's Feature Manager implements the most basic parts of @polishdeveloper's original proposal. This was done because landing a Feature Manager was seen as a priority but a lot of the functionality of the one in MobileFrontend/MinervaNeue (MFE/MN) wouldn't have been used and unused code should be considered a liability. Moreover, collecting usecases and adding functionality (if necessary) should be considered part of the RfC.

I (@phuedx) have documented a couple of simplifications and improvements in T244481#6086600.

The API of the Vector Feature Manager is as follows:

interface Requirement {
    function getName(): string;
    function isMet(): bool;
}

interface FeatureManager {
    function registerSimpleRequirement( string $name, bool $condition );
    function registerRequirement( Requirement $requirement );
    function registerFeature( string $name, string $requirements );
    function isRequirementMet( string $name ): bool;
    function isFeatureEnabled( string $feature ): bool;

Note well that this API can be used to implement @polishdeveloper's proposed solution below with modes being modelled as shared requirements and multi-modal features using some custom requirement, i.e.

$featureManager->registerRequirement(
    new MultiModalRequirement(
        'amc|beta',
        RequestContext::getMain(),
    )
);
$featureManager->registerFeature( 'MultiModalFeature', [ 'amc|beta' ] );

Solution in MFE/MN

The similar work is already done in MobileFrontend extension. Although that code has couple problems that we would like to solve before porting it:

  • ServiceWiring should not depend upon RequestContext::getMain() as we found out that API contexts can be different
  • Too many objects provide isEnabled() method and it's difficult to understand when to use which
  • Code should work as a lib, therefore it shouldn't not assume any environment/request. In order to verify if Feature is available, we need to pass IContextSource to the check method.
  • There is no clear place where the FeatureManager is Initialized, it triggers on the first $mediaWikiServices->getService('FeatureManager') therefore it leads to unexpected behavior when calling hooks
  • Existing solution is pretty verbose, creating single Feature requires us to define name, config variable, group. It provides getNameKey and getDescriptionKey for translations. The Feature interface should be

simple and provide what's required. For example, The feature has no notion of the ResourceLoaderModule/Styles, therefore, when implementing Feature, in one place we had to register it, and in the second place
we had to do if ($featureManager->isAvailableInCurrentContrext( $featureName ) ) { $out->addModules( ... ) }. That is bad, as it doesn't feel like FeatureManager doesn't solve any problem, we could just do
sth like if (!empty(array_filter($config->get( "wg$featureName" )) { $out->addModules(...); } and get almost the same thing.

@polishdeveloper's Original Proposal:

Provide 2 interfaces (FMS stands for FeatureManagementSystem):

  • FMS/IFeature - definition of a single feature
  • FMS/ISet - definition of system mode
  • FMS/IUserSelectableSet set that extends ISet, and additionally can be enabled/disabled by user

and FeatureManager class that binds everything together.

The FeatureManager should provide:

Setters:
registerFeature( $name, IFeature ) to register new feature
registerSet( $name, $ISet ) to register new set
switchSetState( $setName, $newState, IContextSource ) - allow user to switch the set state (only for opt-in/opt-out states )

Getters:
hasFeature( $featureName, IContextSource ) to check if user has access to given Feature
hasSet( $setName, IContextSource ) to check if has access (granted and/or enabled by himself ) to given set
getAllForSet( $setName, IContextSource) to fetch all features assigned to given set
getFeaturesForContext( IContextSource ) to fetch all available features in current context

Additionally it should trigger a hook FeatureManagerInitialization( $featureManager ) where different extensions could register it's own Features/Sets

Built in sets:

  • Stable -> the default mode for all features enabled for all users
  • Loggedin -> the feature set available only for logged-in users

Additionally provide:

  • DesktopImprovements -> the feature set for new DesktopImprovents project

Please, note that some sets, like LoggedIn are granted by system, and other sets like DesktopImprovements have default state that consists of enabled (stored in config) and opted_in stored in UserPreferences.
While implementing the system try to keep it as much compatible with MobileFrontend/FeatureManager as possible. Thanks to that we should move the MobileFrontend features into one system.

The initial delivery should include Legacy and Modern modes. These should be overrideable via a useskinversion URL query parameter (see T242381#5884415) in the feature dependency. The functionality to test query parameters should be available to arbitrary feature dependencies as well. For example, if you wanted to use the existing MobileFrontend feature manager to override the page issues treatment, the URL could be passed as a dependency of the MinervaPageIssuesNewTreatment feature and tested within.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Niedzielski set the point value for this task to 8.Feb 19 2020, 4:01 PM

Estimated asynchronously at an extra-large.

Change 573626 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/skins/Vector@master] features: Add minimalist feature manager

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

@polishdeveloper: Sorry for the lack of context for the change above. There were/are a lot of good questions about the design of the feature management system (FMS) that emerged during review. Given the need to land an FMS in Vector soon, I proposed that we land the simplest version of your proposal that we could all agree on. Included in my patch is a list of steps that gets us from point A to what's described in this task.

Change 573626 merged by jenkins-bot:
[mediawiki/skins/Vector@master] features: Add minimalist feature manager

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

Change 575037 had a related patch set uploaded (by Polishdeveloper; owner: Polishdeveloper):
[mediawiki/skins/Vector@master] AutoloadNamespaces required MW 1.31

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

Change 575037 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Bump required MediaWiki version

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

@Niedzielski @phuedx can this be QAed by Edward or should it move to engineering sign off?

@Jdlrobson, only the initial patch has been merged so far. I believe @phuedx is planning one or more subsequent patches as part of this ticket.

ovasileva added a subscriber: Edtadros.

Change 576122 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/skins/Vector@master] featureManager: Set -> Requirement

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

Change 576123 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/skins/Vector@master] featureManager: Add typehints

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

Change 576124 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/skins/Vector@master] featureManager: Add Requirement interface

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

Change 576125 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/skins/Vector@master] featureManager: Pass context to Requirement::isMet

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

Change 576122 merged by jenkins-bot:
[mediawiki/skins/Vector@master] featureManager: Set -> Requirement

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

@polishdeveloper I've rebased all of the unmerged patches. Thanks for your review!

Change 576123 merged by jenkins-bot:
[mediawiki/skins/Vector@master] featureManager: Add typehints

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

Change 576124 merged by jenkins-bot:
[mediawiki/skins/Vector@master] featureManager: Add Requirement interface

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

Change 579619 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/skins/Vector@master] featureManager: Add FullyInitialised requirement

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

Change 579619 merged by jenkins-bot:
[mediawiki/skins/Vector@master] featureManager: Add DynamicConfigRequirement requirement

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

Change 583441 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/Vector@master] [docs] [dev] [PHP] [FeatureManager] revise docs and add DynamicConfigRequirement test

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

Change 583441 merged by jenkins-bot:
[mediawiki/skins/Vector@master] [docs] [dev] [PHP] [FeatureManager] revise docs and add DynamicConfigRequirement test

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

Change 585049 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/Vector@master] [dev] add skin version query parameter override

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

Change 585291 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/skins/Vector@master] [WIP] Make SkinVector::isLegacy use the Feature Manager

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

Change 585049 merged by jenkins-bot:
[mediawiki/skins/Vector@master] [dev] add skin version query parameter override

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

A brief update, with https://gerrit.wikimedia.org/r/585049 merged, I'm working on making it compatible with the Feature Manager.

Change 576125 abandoned by Phuedx:
featureManager: Pass context to Requirement::isMet

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

Change 585291 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Make legacy mode a feature

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

With the above merged, I'm inclined to move this task towards the end of the process. Thanks to @Niedzielski and @nray for their reviews.

That being said, feature management in Vector isn't Done (neither is it in MobileFrontend and MinervaNeue), as what's been implemented thus far is a remarkably basic version of @polishdeveloper's initial vision. Indeed, I think it can/should be made easier to use in some places and be moved towards the original design in others.

As a reminder, Vector's Feature Manager allows the developer to register requirements and features. Features depend on one or more requirements to be enabled. Both requirements and features must have names.

Simplifications

  1. The config isn't going away any time soon. Adapting the config rather than embracing it only introduces noise. That is, allow requirements to be registered from config variables with a shorthand:
// Registers the shared "CollapsibleSidebar" requirement, which the Feature Manager understands to (lazily?) evaluate by inspecting the global config.
$featureManager->registerRequirement( 'config:CollapsibleSidebar' ); // Or "conf:" or "c:"
  1. Allow features can be registered with unnamed requirements:
$featureManager->registerFeature(
    'LatestVersion', // AKA DIP
    [
      'config:VectorEnableLatest',
      new SkinVersionRequirement( 'latest' ),
    ]
);

Improvements

  1. As in @polishdeveloper's implementation in MFE/MN, there should be a mechanism for registering features between codebases, i.e. a hook
  2. @polishdeveloper remarked that the handling of wgFullyInitialised should be hidden from the developer and I agree. This actually goes hand in hand with the requirement that the Feature Manager should understand that requests to different endpoints can have different request contexts
  3. Make the Feature Manager compatible with MFE/MN's idea of modes either by introducing them explicitly or introducing a convention

I'll update the description tomorrow morning (UTC+1).

phuedx removed the point value for this task.
phuedx set the point value for this task to 8.Apr 28 2020, 5:16 PM

Change 593242 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/skins/Vector@master] [Hygiene] featureManager: ComplexRequirement -> Requirement

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

After ☝️ is merged, this is ready to move to Ready for Signoff. I'll create a task covering the simplifications I've proposed in T244481#6086600 and I'll also take the time to chime in on @polishdeveloper's RfC.

Change 593242 merged by jenkins-bot:
[mediawiki/skins/Vector@master] [Hygiene] featureManager: ComplexRequirement -> Requirement

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

I'm on it, I should sign it off by end of today - so far it looks good but I want to verify couple more things.

polishdeveloper removed polishdeveloper as the assignee of this task.

Great work @phuedx