I just created T243560: Use ObjectFactory to construct ContentHandlers. The way to inject services into a ContentHandler would be similar to what SpecialPageFactory does for EditTags and RevisionDelete: instead of using the plain class name as the "spec", an array is used that specifies the class name and also a list of services to pass. This in this case, the "spec" would be in $wgContentHandlers in DefaultSettings.php.

In T243310#5824718, @Peter.ovchyn wrote:

In T243310#5823600, @daniel wrote:

ContentHandler currently doesn't use LanguageConverter, as far as I can see.

It does! I'm not sure what for but:

This definitely seems like a good idea. The trick is making sure that nothing relies on these fields. We don't have a formal deprecation process for reducing visibility, but it's technically removing something from the public interface, so care should be taken.

In T243213#5819743, @Anomie wrote:

Is there very much difference between global configuration arrays and configuration arrays wrapped in a global service?

In T243213#5818518, @Legoktm wrote:

Make ExtensionRegistry a service.

Last time I tried doing this (never posted to Gerrit) there was some bootstrapping problem where the ExtensionRegistry is needed right away in LocalSettings.php, but that's too early to invoke MediaWikiServices. See rMW69aecc2eeae0: Don't initialize MediaWikiServices before extensions have been loaded as well.

If I understand correctly, the idea is: make it possible to use the MediaWikiServices class in MediaWikiUnitTests, with a mocked instance of MediaWikiServices.

In T214362#5814559, @Addshore wrote:

Yes, we need this!
This decision is one of the 2 rfcs blocking continued work on constraint checks for Wikidata that was started in either late 2018 or early 2019. :)

Tagging Core Platform Team to keep in the loop, and possible provide code review.

This RFC has been approved as proposed per today's TechCom meeting

In T243213#5821616, @Legoktm wrote:

This step seems premature.

I assume this is resolved now.

In T236680#5812296, @Legoktm wrote:

In any case, I think we should have two sets of API tests:

• "structure test" that verify basic API functionality that must always work, e.g. action=help, action=paraminfo, action=query with no other parameters, etc. regardless of what extension is installed. This would be a rather small set of tests that run against both vanilla MediaWiki core and core plus arbitrary extensions.
• "core tests" that do more extensive API testing, but run just against vanilla MediaWiki core instances.

Then there could be a third category of tests, which extensions can extend the core tests and run against core plus arbitrary extensions.

Looks to me like the initial patch is already doing this.

If we want to do this, we should do it soon, before callers of Language::convert are changed to call LanguageConverter::convert.

Would be nice, but I'd push taht back. LanguageConverter knowing about Language isn't a huge problem. Language knowing about LanguageConverter is a lot worse.

This should indeed be done. See T243317#5823612 for the pattern.

DefaultPreferencesFactory is already using DI and is constructed in ServiceWiring.php. Injecting should be trivial.

This should indeed be done. See T243317#5823612 for the pattern.

This lacks context.

I recall making this comment, but I don't recall the context... ContentHandler currently doesn't use LanguageConverter, as far as I can see. Once we have ContentHandlerFactory, we could start to inject all things into ContentHandler subclasses. That would certainly be good. But I don't quite understand what this task is calling for.

Sounds sensible

In T226833#5822347, @Peter.ovchyn wrote:

• Replace and remove Language::newConverter() - Done!
• Move the knowledge encoded in the various implementations of newConverter() into the respective LanguageConverter subclasses - Done!
• Remove $variants, $variantFallback, and similar parameters from the constructor signature of LanguageConverter, hard-coding the values insead. - Done!
• Create LanguageConverterFactory, managed by MediaWikiServices. - Done!
• Hard-code a mapping from language code to LanguageConverter subclass in * LanguageConverterFactory. - Working On Right Now!
• Make the LanguageConverter base class abstract. - Done!

In T243051#5820582, @Anomie wrote:

If the plan isn't to replace getQueryInfo(), we can leave this point until then. When we do come back to it, the issue IMO is more in "combining" than "filling".

In T243051#5818516, @tstarling wrote:

Is it easier to accept a context-sensitive useIndex() function if you consider it to be effectively appending to a query string?

Tagging TechCom for discussion, since this seems like a cross-cutting concern.

In T243051#5815711, @tstarling wrote:

How about

$queryBuilder ->table( $tableName, $alias )->useIndex( $index )->ignoreIndex( $otherIndex );

Where useIndex() and ignoreIndex() act on the most recently declared table? It's implicit but brief, and you could be creative with line breaks to help readability. Note that there is also $queryBuilder->options() as a last resort array hacking function, so it's never necessary to parse an options array and convert it to a call chain.

In T236680#5811357, @Jdforrester-WMF wrote:

In T236680#5811337, @WDoranWMF wrote:

@hashar Is there a mechanism that would allow for disabling tests for extensions but not core as @daniel suggests?

No. (And I would oppose attempts to do such a thing; the whole point of CI is to see what tests from which repos are broken by which other repos.)

In T214362#5808924, @Addshore wrote:

I see that T227776 has had little activity since November, is there any way to move this forward?

On the subject of using php interfaces, as briefly discussed on slack: I'm not opposed to introducing IContentHandler, but I don't see much value in it. I do see the value in having a narrow IContentHandlerFactory interface that is implemented by ContentHandlerRegistry.

Condensed outcome of a conversation between @Ladsgroup, @Addshore, @Joe, @Krinkle, @tstarling, and myself:

In T233520#5805312, @Addshore wrote:

Adding core platform team to get some feedback and figure out:

• When exactly this change in behaviour happened?
  • I have looked through various gerrit changes and code paths but i'm sure someone that has worked on this code would be able to figure out exactly when this happened.
  • It's slightly confusing as some stages look like a partial implementation, in some places, that may not have worked as expected etc.

In T99268#5808193, @srishakatux wrote:

@WDoranWMF @kchapman @daniel Would you all be interested in promoting/mentoring this project via Google Summer of Code 2020 or Outreachy Round 20?

In T225197#5806883, @Krinkle wrote:

Those two lines using thumb are the only two in the codebase that use that channel. The previous one being introduced in 2015 and never enabled in prod from what I can see. There is however a more common thumbnail channel enabled in prod.

We should probably disable the integration tests for extensions for now.

In T226833#5805009, @Peter.ovchyn wrote:

By the way - our deployment cycle is weekly, but the release cycle is twice a year. The next release would be some time in the summer.

Can we remove these lines then:

In T236680#5805866, @CCicalese_WMF wrote:

Can the tests be run with a bot password?

In T225197#5804063, @Krinkle wrote:

@daniel Remember to enable the thumb channel in production, or, given this is probably temporary instrumentation, perhaps use AdhocDebug instead (and cherry-pick to wmf, leave unmerged in master).

I haven't done a full analysis, but I'm wondering why this is using the ContentAlterParserOutput hook, instead of RevisionDataUpdates? Conceptually, ContentAlterParserOutput is a filter-style hook that shouldn't have side effects. RevisionDataUpdates is specifically designed for recording information into the database after an edit.

In T226833#5803628, @Peter.ovchyn wrote:

1. I left mutual dependency for now as I have to make sure about the goal. Therefore LanguageConverterFactory::createFromCode

has got weird signature:

public function createFromCode($code, \Language $language)

In T236680#5804582, @Legoktm wrote:

So far all the failures are assumptions about permissions or login, which are fully compliant with the API and applications should be coded around that. Can those tests be disabled/fixed?

If these tests are just for core, can they just run against mediawiki/core patches and not extensions?

Might be relevant in this context: T220608: Introduce UnknownContentHandler and UnknownContent

In T211450#5802204, @828bmartin wrote:

Is there any way to get around this issue? I'm currently trying to import an XML dump into a local MediaWiki install but getting this error.

The sole remaining issue here is to add a test for UserMerge that tests the page move, so we can merge the respective patch with confidence:

@Somebody please use the "Edit task" link at the top of the page to update the task description. Your description is good, but it should be at the top, not in a comment down here.

Looks like this boils down to wfGetLangObj() returning $wgLang, which has a stale Language object. The same stale object would also be contained in the main RequestContext.

Thanks for testing @jcrespo, I'll check it out!

Hi Esther!

This RFC has been put on Last Call per the TechCom meeting on January 8. If there are no issues left unresolved by January 22, this RFC will be approved as proposed.

In T241638#5790036, @jcrespo wrote:

Out of curiosity, do you know why specifically with Turkish and not with other (non-English) languages?

In T241638#5790036, @jcrespo wrote:

Not the original reporter, but after applying the patch I can confirm the turkish installation went through correctly (twice):

Putting on the TechCom board for implementation. That is, putting the document in the correct place on the wiki.

In T240307#5786336, @Anomie wrote:

I like it. Simple and straightforward while still covering the necessary use cases.

In T240307#5784843, @tstarling wrote:

Task description edit: remove the term "listener" and move hook interfaces into the namespace of the caller.

The instances of this I currently see on Kibana all come from FlaggedRevs:

I made a patch for the 1_34 branch, https://gerrit.wikimedia.org/r/c/mediawiki/core/+/562510