Problem/Motivation

Currently, the module page shows SQL queries to execute for unbanning.

Having dedicated 

ban:unban:all
ban:unban:ip

commands would be better DX and should be self-explaining.

As flood_control already has this for its own tables, this should be a simple novice task to do the same for the ban tables:
https://git.drupalcode.org/project/flood_control/-/blob/3.0.x/src/Comman...

Steps to reproduce

Try unbanning an IP using drush - currently needs custom SQL commands.

Proposed resolution

Implement

ban:ban:{IP}
ban:unban:{IP}
ban:flush

drush commands like flood_control does.

Remaining tasks

Implement
Test
Release

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#11 3553606-add-drush-unban-commands.patch9.35 KBysamoylenko

Issue fork ban-3553606

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

anybody created an issue. See original summary.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Issue summary: View changes
danrod’s picture

danrod
Location Ottawa
commented

I'd like to work on this one if you don't mind.

danrod’s picture

danrod
Location Ottawa
commented
Assigned: Unassigned » danrod
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

@danrod Yes of course, that would be great! Any updates yet? :)

danrod’s picture

danrod
Location Ottawa
commented

Hi @anybody , It's ongoing, I've had some health issues lately, hopefully I'll continue working with this today.

lrwebks’s picture

lrwebks commented
Assigned: danrod » lrwebks
lrwebks’s picture

lrwebks commented

@danrod, definitely hope you get well soon! @anybody assigned this task to me in the meantime, hope you don't mind!

lrwebks opened merge request !4

danrod’s picture

danrod
Location Ottawa
commented

Hi @lrwebks thanks, yes, it is ok, hopefully I'll be able to check some Drupal issues this weekend.

ysamoylenko’s picture

ysamoylenko commented
Status: Active » Needs review
StatusFileSize
new 3553606-add-drush-unban-commands.patch9.35 KB

This patch implements the requested Drush unban commands plus additional commands for a complete Drush interface to the Ban module.

What's Included:
- ban:unban:ip - Unban a specific IP address (as requested)
- ban:unban:all - Unban all IP addresses (as requested)
- ban:add - Ban an IP address (bonus feature)
- ban:list - List all banned IPs (bonus feature)

All commands include:
✓ Modern Drush 13 patterns using PHP 8 attributes
✓ Proper dependency injection with AutowireTrait
✓ Multiple aliases for user convenience
✓ IP address validation
✓ Comprehensive error handling
✓ Clear user feedback messages

Testing:
✓ Complete functional test suite (6 test methods)
✓ All tests cover success and error cases
✓ Tests verify command aliases work correctly
✓ Edge cases including IPv6 support

Code Quality:
✓ Zero PHPCS violations (Drupal & DrupalPractice standards)
✓ Zero syntax errors
✓ Full type declarations
✓ Complete PHPDoc documentation
✓ 100% backwards compatible

The patch applies cleanly to the 1.0.x branch and is ready for review.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Needs work

@ysamoylenko please use MRs not patches. It's 2025 ;D

ysamoylenko changed the visibility of the branch 1.0.x to hidden.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: lrwebks » Unassigned
Status: Needs work » Needs review

I guess the broken tests are from #3540366: Implement phpstan level 8 and unrelated. So this should be reviewed.

grevil’s picture

grevil commented
Status: Needs review » Needs work

Commented.

ysamoylenko’s picture

ysamoylenko commented
Status: Needs work » Needs review
grevil’s picture

grevil commented
Status: Needs review » Reviewed & tested by the community

Thanks, @ysamoylenko! LGTM! 👍

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

@mstrelan any chance to merge this and release next? This would be *REALLY* helpful!

mstrelan’s picture

mstrelan commented

Have made you a maintainer. Let's open a 1.1.x branch for this and new features, and if we ever do a big restructure / merge with advban etc we can open 2.x

grevil’s picture

grevil commented

I created a new 1.1.x Branch, but I don't have the permissions to add the Branch as a dev release, hence I can't target 1.1.x here.

grevil’s picture

grevil commented
Status: Reviewed & tested by the community » Needs work

And one more thing, maybe we should slightly adjust the command names. It feels quite intuitive to have ban:unban:ip xxx to unban, but ban:add xxx to ban an ip.

IMO we should either have:

ban:ban:x
ban:unban:x

or:

ban:add:x
ban:remove:x

Also ban:ban:ip and ban:unban:ip could also support multiple ips?

What do you think @anybody.

mstrelan’s picture

mstrelan commented
Version: 1.0.x-dev » 1.1.x-dev
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

@grevil consolidating the commands is a good idea. Let's do the following:

ban:ban:IP
ban:unban:IP
ban:flush (removes all), alternatively ban:unban-all
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Issue summary: View changes
grevil’s picture

grevil commented

SGTM!

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: Unassigned » grevil
grevil’s picture

grevil commented
Assigned: grevil » Unassigned
Status: Needs work » Needs review
Issue tags: -Novice

All done! Please review!

grevil’s picture

grevil commented

I'd keep the aliases. Anything else?

grevil’s picture

grevil commented
Status: Needs review » Needs work

Ah tests fail.

grevil’s picture

grevil commented
Status: Needs work » Needs review

I went back and forth, whether to keep or remove the tests. They are ok, but only test the underlying BanManager. Only 3 tests test the commands but only the list command in three ways.

We already have middleware tests, which indirectly test the BanManager. We can open a separate issue for further tests, but I don't want to merge them here.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Reviewed & tested by the community

Nice @grevil all good, thanks! Merged!

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

mstrelan’s picture

mstrelan commented

Similar comment here regarding BC. Adding to BanIpManagerInterface means all implementations need to add the new method. It is probably unlikely that people are implementing this without also extending BanIpManager, so it's probably ok, but can't say for sure. In saying that, I'm not sure exactly how to achieve this in a BC way.

grevil’s picture

grevil commented

I'll fix the introduced breaking changes in #3575604: Remove introduced breaking changes in 1.1.x.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Parent issue: » #3575766: [META] 1.1.x UX-Improvements

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.