Page MenuHomePhabricator

Use autoloader more
Closed, DeclinedPublic

Description

CirrusSearch uses various requires, mostly in maintenance related files... It should really register things more in the autoloader... Including abstract classes :)

require_once( __DIR__ . '/../includes/Maintenance/Maintenance.php' );

Event Timeline

Reedy created this task.Aug 10 2016, 10:48 PM
Restricted Application added projects: Discovery, Discovery-Search. · View Herald TranscriptAug 10 2016, 10:48 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

UpdateOneSearchIndexConfig is another that is require'd in various places and should just be autoloaded

debt triaged this task as Normal priority.Aug 11 2016, 4:47 PM
debt moved this task from needs triage to later on... on the Discovery-Search board.
debt added subscribers: EBernhardson, debt.

We think this would be a tremendous amount of work to do, just FYI.

Adding this ticket as a related ticket: T142652

@Reedy: My understanding is that this ties to a larger initiative. It would be great if you could add a couple links here to emails or wiki pages describing that work. That would help someone reading this task understand the context and priority.

Reedy added a comment.Aug 11 2016, 7:31 PM

We think this would be a tremendous amount of work to do, just FYI.
Adding this ticket as a related ticket: T142652

I think you've got it backwards. By *not* using the autoloader, you're creating more work and a higher maintenance burden for yourselves. Granted, you've already done it now, but that's not a good reason to not fix it.

Just having the autoloader, means you don't need to be including all the random files and classes you want to use. It's been helping for a long time.

@Reedy: My understanding is that this ties to a larger initiative. It would be great if you could add a couple links here to emails or wiki pages describing that work. That would help someone reading this task understand the context and priority.

Not particularly, it's just good practice? It's useful, but not a blocker for extension registration, and general tech debt cleanup that should usually be an ongoing process, as it should with any software development project.

The conversion to extension registration project is a bigger effort, which needs to be done; allowing further cleanup of other tech debt such as the way we find localisation files, unit tests etc. Stuff that really shouldn't be hardcoded, but it is. You can see that through T87892, T87875, T98668 and https://www.mediawiki.org/wiki/Extension_registration

The majority of WMF deployed extensions have been converted, as of .14, there's 19 extensions left to convert, 125 done, and between 5 and 10 with WIP patches...

I think you've got it backwards. By *not* using the autoloader, you're creating more work and a higher maintenance burden for yourselves. Granted, you've already done it now, but that's not a good reason to not fix it.

I'm sure everyone here is aware of that. Still, the fact is that this is extra work, and needs to be prioritised accordingly. Nobody is proposing to not do this, it's just a question of when. If the team's not quite ready to make that tradeoff yet, then they'll get to it when they are.

The conversion to extension registration project is a bigger effort, which needs to be done; allowing further cleanup of other tech debt such as the way we find localisation files, unit tests etc. Stuff that really shouldn't be hardcoded, but it is. You can see that through T87892, T87875, T98668 and https://www.mediawiki.org/wiki/Extension_registration

Thanks for the links. :-)

Reedy added a comment.Aug 11 2016, 7:40 PM

I think you've got it backwards. By *not* using the autoloader, you're creating more work and a higher maintenance burden for yourselves. Granted, you've already done it now, but that's not a good reason to not fix it.

I'm sure everyone here is aware of that. Still, the fact is that this is extra work, and needs to be prioritised accordingly. Nobody is proposing to not do this, it's just a question of when. If the team's not quite ready to make that tradeoff yet, then they'll get to it when they are.

Aha. It's "important", but it's certainly not of any high priority, or really a blocker to anything (yet?)

Deskana added a comment.EditedAug 11 2016, 7:45 PM

Aha. It's "important", but it's certainly not of any high priority, or really a blocker to anything (yet?)

Almost everything is important at some point, otherwise it wouldn't have a task filed for it. :-)

You are correct that this is not presently being viewed as high priority since it doesn't actively block anything. I trust my engineers to do the appropriate research and make determinations on when this can be picked up.

The reason for maintenance classes being required is that MediaWiki has a very backwards way of loading maintenance scripts. When the class file is read in no autoloader exists yet, that happens as part of doMaintenance.php which is required at the bottom of the file.

Many months ago I looked into reworking how core loads maintenance scripts but it would take a good amount of work without any direct payoff beyond making engineers feel better about the loading process.

EBernhardson closed this task as Declined.Feb 14 2019, 9:26 PM