Page MenuHomePhabricator

Avoid using UserIdentity::getUserId. Use UserIdentity::getId instead.
Closed, ResolvedPublic

Description

Remove UserIdentity::getUserId. Use UserIdentity::getId instead.

Event Timeline

vadim-kovalenko renamed this task from Removing UserIdentity::getId to Avoid using UserIdentity::getUserId. Use UserIdentity::getId instead..Feb 23 2021, 12:16 PM
vadim-kovalenko updated the task description. (Show Details)

Change 666411 had a related patch set uploaded (by Vadim Kovalenko; owner: Vadim Kovalenko):
[mediawiki/core@master] Avoid using UserIdentity::getUserId

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

Change 666411 merged by jenkins-bot:
[mediawiki/core@master] Avoid using UserIdentity::getUserId

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

Change 667991 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/extensions/GrowthExperiments@master] Replace getUserId() with getId()

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

Change 668033 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[mediawiki/core@master] Re-introduce UserIdentity::getUserId - removed in a violation of SIP

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

This broke two extensions (c667991, c667974). Please be more careful in the future when making breaking changes.

Change 667991 merged by jenkins-bot:
[mediawiki/extensions/GrowthExperiments@master] Replace getUserId() with getId()

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

This broke two extensions (c667991, c667974). Please be more careful in the future when making breaking changes.

Thanks for pointing this out @Tgr - I'm here because SecurePoll was one of those extensions.

I appreciate that getUserId wasn't covered by the stable interface policy yet, but wondering what we should do to avoid this in the future... Should extension authors to keep using the older, deprecated method until the new one has been included in a stable release, or should whoever removes the new method check for extension uses first?

Change 668033 merged by jenkins-bot:
[mediawiki/core@master] Re-introduce UserIdentity::getUserId - removed in a violation of SIP

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

Pchelolo added a subscriber: Pchelolo.

The fix has been partially reverted due to breaking extensions. https://gerrit.wikimedia.org/r/c/mediawiki/core/+/668033

We need to find all the usages in the extensions before removing methods. Let's try again.

I appreciate that getUserId wasn't covered by the stable interface policy yet, but wondering what we should do to avoid this in the future... Should extension authors to keep using the older, deprecated method until the new one has been included in a stable release, or should whoever removes the new method check for extension uses first?

Removing deprecated methods happens all the time. Unding an interface change is pretty rare. So the latter makes more sense IMO.

We need to find all the usages in the extensions before removing methods. Let's try again.

FWIW I did check via codesearch and gerrit earlier and didn't found any other usage than GrowthExperiments and SecurePoll (both fixed now) so I don't think a revert is necessary.

Change 669824 had a related patch set uploaded (by Vadim Kovalenko; owner: Vadim Kovalenko):
[mediawiki/core@master] Avoid using UserIdentity::getUserId

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

Change 669836 had a related patch set uploaded (by Vadim Kovalenko; owner: Vadim Kovalenko):
[mediawiki/extensions/AbuseFilter@master] Replace UserIdentity::getUserId with ::getId

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

Change 669836 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Replace UserIdentity::getUserId with ::getId

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

Change 669824 merged by jenkins-bot:
[mediawiki/core@master] Avoid using UserIdentity::getUserId

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

What was the reasoning for this back and forth? I can't find much of an explanation anywhere.

getUserId() was introduced and getId() deprecated in https://gerrit.wikimedia.org/r/658751 in 2021-02-02, linked to T260933: Make UserIdentity objects aware of which wiki they belong to. This ticket sounds like it is unrelated. Why is that?

The deprecation of getId() was finally undone in https://gerrit.wikimedia.org/r/669824 in 2021-03-10, linked to this ticket here. Again with no explanation.

Personally I agree that getId() is the better choice for something that's already called UserIdentity. Not only that, it's used thousands of times in existing code. Changing e.g. $user->getId() to $user->getUserId() doesn't really make anything better.

What was the reasoning for this back and forth? I can't find much of an explanation anywhere.

You are right, trying to change it was a mistake. I think now the mistake was corrected and we fully rolled it all back, so I'll resolve this ticket.

The logic behind the original switch was that UserIdentity has been initially named incorrectly. In reality it's not UserIdentity, but ActorIdentity. We were thinking to rename it, in which case the getId() in its current form would become very odd, but then decided against that and rolled back all the changes.

Ah, I see. That's really helpful to know. Thanks a lot!