Page MenuHomePhabricator

[Spike] Explore different options for code sharing of page-specific configuration between Minerva and Vector
Closed, ResolvedPublic5 Estimated Story PointsSpike

Description

Background

To implement T359606 we need to duplicate PHP code in Minerva inside the Vector skin. This is the first time we have encountered this problem, and we decided it was important to pause and think carefully about how to do this for resolving this issue, but also to provide clarity in future work.

The code in question provides a specific type of configuration which allows us to make decisions about pages on a per request basis. For example: we might want to only limit code to certain pages e.g. diff page/main page.

We currently have this code pattern in several places. For example we:

The three options to be considered are:

  • Duplicating the code and maintaining 2 identical sets of code
  • Sharing code via composer plugin
  • Sharing code via MediaWiki core.

In response to the feedback and the need for a refined approach to managing night mode settings across different skins, particularly between Minerva and Vector 2022, this amendment aims to enhance the clarity and direction of the development process, ensuring that the objectives are met efficiently and effectively.

TODO

  • Identify the three options and pros and cons of each
  • Write a draft ADR with status "in progress" defining the outcome of the spike.

Mo's analysis

Code Duplication Minimisation:

It is imperative to reduce code duplication to the greatest extent possible. Future modifications to night mode functionality should not necessitate parallel maintenance efforts. By streamlining the codebase, we aim to enhance maintainability and facilitate easier updates.

  1. Utilising a Composer Library for Shared Options: This strategy involves creating or utilising an existing Composer library that contains the shared functionalities, making it accessible across different skins.
    • Pros: This method promotes code reuse and maintainability, ensuring that any updates or bug fixes are centralised, thus reducing the overhead in managing similar code across multiple skins. It could also enhance the consistency of features like night mode across different user interfaces.
    • Cons: However, this approach may introduce dependencies on external libraries, which could potentially affect the deployment process and require additional maintenance. It also necessitates a well-designed interface for the shared library to ensure it meets the diverse needs of each skin it supports. In addition to that it needs to wait for the security team queue which is a long one.
  2. Moving Code to Core MediaWiki: This approach involves integrating the shared code directly into the core MediaWiki software. This also means it have to be generic and more adaptable tho things other than skins and probably be called ConfigDecisionEngine and will be a bigger refactor
    • Pros: By moving the relevant code to the core, it ensures that the functionality is available globally across all skins and extensions, without the need for duplication. This can streamline the development process and facilitate easier updates and bug fixes.
    • Cons: On the downside, incorporating too much skin-specific functionality into the core can bloat the core codebase and potentially slow down the system. It also requires careful consideration to avoid introducing features that are too specialised for a general-purpose core.
  3. Duplicating Code in Skins: This option means maintaining separate copies of the code for each skin that requires the functionality.
    • Pros: Code duplication allows for faster, skin-specific customisations without worrying about the impacts on other skins or the need for extensive testing across different environments. It provides a high degree of flexibility for skin developers.
    • Cons: The major drawback of this approach is the increased maintenance effort. Bugs fixed or improvements made in one skin's code need to be manually replicated across others, which is inefficient and error-prone. Over time, this can lead to inconsistencies and a fragmented user experience.

Event Timeline

@ovasileva - to tie this to relevant user story ticket

ovasileva set the point value for this task to 5.

Estimating as a 5 to account for exploring the possibility of sharing approach with mobile

Change 997498 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[mediawiki/skins/Vector@master] Vector Allow pages to not appear in night mode

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

https://github.com/moabualruz/SkinsCommon I create this new repo, as I do not have permission to create a new one in gerrit until we can get that done, I am only addressing the reduce duplicate code points, please let me know if this is not the intended scope

ovasileva lowered the priority of this task from High to Medium.Feb 29 2024, 5:54 PM

@Mabualruz the use of a composer library is in scope but doesn't necessarily need to be completed as part of this sprint but it must be discussed.

The important thing here would be to:

  • present the options to the team and the associated trade offs (google doc)
  • Facilitate discussion either async in the document or in person
  • Collect a consensus about how we want to approach this.
  • Document decision in an decision record (ADR) in the patch.
Jdlrobson updated the task description. (Show Details)

Composer Registration in packagist.org requires to be a maintainer of mediawiki so we will use mo-mediawiki/skins-common until We publish it officially, or I get maintainer access

Change 1008908 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[mediawiki/core@master] Vector Allow pages to not appear in night mode

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

Change 1008909 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[mediawiki/skins/MinervaNeue@master] Minerva use FeaturesHelper from core

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

Mabualruz updated the task description. (Show Details)

@Mabualruz the use of a composer library is in scope but doesn't necessarily need to be completed as part of this sprint but it must be discussed.

The important thing here would be to:

  • present the options to the team and the associated trade offs (google doc)
  • Facilitate discussion either async in the document or in person
  • Collect a consensus about how we want to approach this.
  • Document decision in an decision record (ADR) in the patch.

I don't think there's an inherent issue with using a composer library in itself (though there might be easier solutions here for the short-term as we look into a longer-term one?)

But I will caution that it seems like the current code heavily references mediawiki, which is not something we usually do in vendor libraries. Vendor libraries are standalone; libraries are not supposed to *require* mediawiki. It seems like a problematic structure that may get you more long-term trouble than you're signing up for, especially during train deployments where any sort of bug will be opaque and hard to trace for train conductors, and will have other added risks.

I would not make this a composer library if you need behavior that's inherent MW internals. We can see if you can add a module into core's "frontend" area, potentially, or some other viable solution as we discuss a more long-term viable one. Otherwise, it would be best if you made the vendor library MW-agnostic -- have it run the business logic without knowing or expecting a reach into the internals of mw -- and then you can use that.

We can talk more about the options that we can utilize short- and potentially longer term. I think one of the things here is that you're identifying the need for a pattern that is missing in the current platform and you're not the only ones that need this pattern. It might be a good opportunity to flesh out what kind of pattern/capability we should look into potentially enabling in mw core long-term (but then finding the short-term solution so you can release the feature while we work on a deeper solution).

Either way -- please make sure a vendor library is not requiring mediawiki, and does not reach into mediawiki's internals.

@Mabualruz to summarize where we are in an ADR and move to sign off. I'll make some edits to the ticket and spin out an epic later today.

Change 1009394 had a related patch set uploaded (by Jdlrobson; author: Mabualruz):

[mediawiki/skins/Vector@master] ADR - Code sharing between Vector and Minerva Skins

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

Jdlrobson renamed this task from It should be possible to disable night mode on select pages in Vector 2022 to [Spike] Explore different options for code sharing between Minerva and Vector.Mar 8 2024, 12:19 AM
Jdlrobson updated the task description. (Show Details)
Jdlrobson renamed this task from [Spike] Explore different options for code sharing between Minerva and Vector to [Spike] Explore different options for code sharing of page-specific configuration between Minerva and Vector.Mar 8 2024, 12:40 AM
Jdlrobson updated the task description. (Show Details)

Change 1009394 merged by jenkins-bot:

[mediawiki/skins/Vector@master] ADR - Code sharing between Vector and Minerva Skins

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

Jdlrobson added a subscriber: SToyofuku-WMF.

I have revised this based on my understanding of the new scope and have created T359607 to continue the good work here. Both of these are subtasks of the original ticket (which is now T359606).

While working on this I also discovered https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/1009628 which caused confusion to me, so I think we should clean that up before talking to other people.

@SToyofuku-WMF could you please check that everything I've said here makes sense and resolve this ticket if it does? Thanks in advance!

Everything here makes sense!! Thank you to all involved for being so thoughtful and flexible as we work through this ☺️

Restricted Application changed the subtype of this task from "Task" to "Spike". · View Herald TranscriptMon, Apr 1, 9:52 PM