Page MenuHomePhabricator

BlockRestriction should be a service instead of a singleton
Closed, ResolvedPublic3 Estimated Story Points

Description

Problem
The BlockRestriction class was created as a singleton instead of a service. There isn't a reason why this shouldn't be a service which is consistent with the (proposed) best practices.

Solution
Convert to a service and make static methods non-static.

Event Timeline

Niharika subscribed.

@dbarratt Reading this ticket it isn't obvious why making it a service is better and who's it gonna affect, if anyone. It'd be nice to have that documented in the ticket.

Niharika triaged this task as Medium priority.Apr 4 2019, 6:10 PM
Niharika set the point value for this task to 3.

What is the plan here exactly? Is there any deprecation process that we have to go through?
Do we want to rename the BlockRestriction class to something else?

I think it would be safe to do any changes we want without deprecating since our team recently added this feature and it is very unlikely that it is being used outside of the block changes we did. A quick look at codesearch does not reveal any use of it outside of block related pages. With that said, making changes to a public interface without deprecating first violates MW policies

Is there any deprecation process that we have to go through?

I think it's fine to not deprecate it at all, but if you want to be nice you could leave the name the same and the methods the same and just throw the deprecation notice if the method is called statically.

Do we want to rename the BlockRestriction class to something else?

Up to you. :)

It sounds like there's not a lot of consensus with this pattern (correct my assumption here if I've misread the other conversations) so I think we should leave the name as it is.

I think David's approach given the lack of use of this class in the code is a good compromise.

@dmaza Also, any dependencies (config, database, etc.) should be injected rather than making the service aware of the container.

When did we introduce this? If we introduced it in a previous (and published) release, we might need to deprecate for the upcoming one? ... But if it's in between releases it's probably fine.

Either way, the deprecation should be super straight forward even if we do it because, as you pointed out, no one uses it this way outside our code.

@dmaza Also, any dependencies (config, database, etc.) should be injected rather than making the service aware of the container.

That's the plan. I was just wondering about all the different names we are currently working with and I wanted to end up with a name that we are all happy with since renaming is a bit more complicated and this is the time to do it if any. But..

I think David's approach given the lack of use of this class in the code is a good compromise.

agreed

I think it's fine to not deprecate it at all, but if you want to be nice you could leave the name the same and the methods the same and just throw the deprecation notice if the method is called statically.

Let's soft deprecate the static usage. It is nice and follows our MW Deprecation policy

Let's soft deprecate the static usage. It is nice and follows our MW Deprecation policy

I don't think there's a reason to not hard-deprecate at the same time. Unless you're planning on leaving the usage in core?

When did we introduce this? If we introduced it in a previous (and published) release, we might need to deprecate for the upcoming one? ... But if it's in between releases it's probably fine.

Either way, the deprecation should be super straight forward even if we do it because, as you pointed out, no one uses it this way outside our code.

A while back https://gerrit.wikimedia.org/r/c/mediawiki/core/+/453081

See my previous comment. Let's deprecate as @dbarratt suggested

Oh... it looks like it hasn't been in a released version yet... so if that's the case, no deprecation needed. :)

Oh... it looks like it hasn't been in a released version yet... so if that's the case, no deprecation needed. :)

The 1.32 release branch is being cut today though, so if your plan is to make this change and backport it to REL1_32 before the final release of 1.32, you should probably do that relatively quickly. (I don't know exactly what the timeline is for a release, but it's probably a few weeks.)

Change 503098 had a related patch set uploaded (by Dmaza; owner: Dmaza):
[mediawiki/core@master] Rename BlockRestriction -> BlockRestrictionStore and wire it up as a service

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

Change 503098 merged by jenkins-bot:
[mediawiki/core@master] Rename BlockRestriction -> BlockRestrictionStore and wire it up as a service

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

Change 506851 had a related patch set uploaded (by Jforrester; owner: Dmaza):
[mediawiki/core@REL1_33] Rename BlockRestriction -> BlockRestrictionStore and wire it up as a service

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

Change 506851 merged by jenkins-bot:
[mediawiki/core@REL1_33] Rename BlockRestriction -> BlockRestrictionStore and wire it up as a service

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

dmaza moved this task from Review to Done on the Anti-Harassment (Teth - ט) board.