Page MenuHomePhabricator

PageContentAccess service as a replacement for WikiPage::getContent
Open, Stalled, MediumPublic

Description

One common use case for WikiPage is accessing the current revision's content. If we want to stop passing around instances of WikiPage, and instead pass around PageIdentity or PageRecord, we need an easy way to access the current page content.

Interface draft:

interface PageContentAccess {

    public function getPageContent( PageIdentity $page, Authority $authority = null, $role = SlotRecord::MAIN ): ?Content;
}

The implementation can essentially be a wrapper around RevisionStore::getRevisionByTitle()

Event Timeline

daniel lowered the priority of this task from High to Medium.Feb 10 2021, 10:25 AM
daniel raised the priority of this task from Medium to High.Mar 9 2021, 6:28 PM
daniel lowered the priority of this task from High to Medium.Aug 17 2021, 3:41 PM

Change 724858 had a related patch set uploaded (by D3r1ck01; author: Derick Alangi):

[mediawiki/core@master] page: Introduce a `PageContentAccess` service to replace WikiPage::getContent

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

@daniel, I'm thinking we should call the service AccessPageContent instead so it reads well on the first go. But we can also leave it as is but I just wanted to understand your inspiration behind the service name.

I've proposed a patch to add this service and I'll also survey callers of WikiPage::getContent() within core and make updates as required (though I've updated all usage in WikiPage.php). Let me know if AccessPageContent makes sense to you. ;}

DAlangi_WMF changed the task status from Open to Stalled.Thu, Sep 30, 4:54 PM

@Pchelolo dropped a comment in the patch about the scope of this and there is some work happening in ContentHandler which will touch this so let's wait a little.

@daniel, I'm thinking we should call the service AccessPageContent instead so it reads well on the first go. But we can also leave it as is but I just wanted to understand your inspiration behind the service name.

"AccessPageContent" would be a verb phrase... Generally, classes names should be nouns. I don't really like "PageContentAccess" though, I just couldn't think of anything better at the time. PageContentFetcher? Something like that...

Copying my response from gerrit:

Honestly, I donno. The point of the service is 'get latest content for the page', e.g. $revStore->getRevisionByPage( $page )?->getContent()'

I see that this method is highly used, but there's a lot of highly used methods on WikiPage - are we going to introduce a service for each method?

Also, I'm not sure that getting the latest content from a page (when we skip loading a revision) actually should be such a common operation. Now a lot of methods on Content are misplaced, like, getUltimateRedirectTarget for example. When these methods are moved away, the need to get the Content object will decrease. Let's postpone this. It's too narrow scoped for making it into a service.

TLDR: let's wait and see where ongoing Content refactor brings us before introducing this new service.

TLDR: let's wait and see where ongoing Content refactor brings us before introducing this new service.

I think we want an abstraction like this, but perhaps it's not this one. Surveying callers should bring more clarity.