Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SiteLinkLookup from Wikibase Lib #60

Closed
wants to merge 1 commit into from
Closed

Conversation

Benestar
Copy link
Contributor

@Benestar Benestar commented Sep 1, 2015

No description provided.

@JeroenDeDauw
Copy link
Contributor

$pageTitle is a MediaWiki concept. Should not be in this component

@Benestar
Copy link
Contributor Author

Benestar commented Sep 2, 2015

That's just the naming of the variable. It's obvious that the method is just another version of getItemIdForSiteLink. We can however adjust the naming of the variables. That doesn't really change the interface though.

@addshore
Copy link
Contributor

addshore commented Sep 2, 2015

We could just kill the getItemIdForLink in the interface?

@Benestar
Copy link
Contributor Author

Benestar commented Sep 2, 2015

Even better idea @addshore #killallthethings

@addshore
Copy link
Contributor

addshore commented Sep 2, 2015

hah, I think I am getting a name for myself when it comes to killing things...
Either kill it or rename the params to $siteId and $pageName as in https://github.com/wmde/WikibaseDataModel/blob/master/src/SiteLink.php#L47

@Benestar
Copy link
Contributor Author

Benestar commented Sep 2, 2015

/me is looking for usages

@Benestar
Copy link
Contributor Author

Benestar commented Sep 2, 2015

We should still have the method in there. There are cases where we deal with title objects and site ids independently and we perhaps don't want to construct a SiteLink object for no reason.

@JeroenDeDauw
Copy link
Contributor

I'm looking through the git history for this interface out of curiosity. Originally it just had

public function getConflictsForItem( Item $item );
public function getItemIdForLink( $globalSiteId, $pageTitle );

First degradation happened in https://gerrit.wikimedia.org/r/#/c/32172/4/lib/includes/store/SiteLinkLookup.php by me :) After that it grew more and some stuff got removed

@JeroenDeDauw
Copy link
Contributor

I'm thinking this interface is probably not well segregated, and should be split based on the users of the methods. While for the initial move from WB.git we figured to just move and improve later, I suspect it's easier now to first improve and then move. This avoids forcing breaking releases of this component and should be easy enough to manage, as we are not moving dozens of things and in the process of introducing new component.

Does someone know if any of the interfaces methods are frequently used together, or used together at all?

* The links are returned as arrays with the following elements in specified order:
* - string siteId
* - string pageName
* - int itemId Numeric (unprefixed) item id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a weird interface. An array with three numerically indexed elements? I do not think such an interface should be in this generic services component. This sounds like a concept that's specific to Wikibase.git and should stay there.

@JeroenDeDauw
Copy link
Contributor

No changes since 2015. Please reopen if needed for current goals

@JeroenDeDauw JeroenDeDauw deleted the sitelinklookup branch August 8, 2018 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants