Problem/Motivation

Drupal 12 just got its own built-in CLI tool dr, maybe we can add support for that in Ban?

Ban could be the first Drupal contrib module with dr support, isn't that something to strive for?

Steps to reproduce

Run dr and not see Ban commands listed.

Proposed resolution

Expand the Ban module to support the new dr CLI tool, so that the "Ban Drush Commands" from the project page also work with dr.

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork ban-3606943

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

ressa created an issue. See original summary.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

IMHO dr and drush should simply be able to use the same commands instead of implementing that twice. Does such a plan exist?

ressa’s picture

ressa
he/him
commented

I agree, the less overlap in the code the better, and I asked for help from @moshe weitzman in the Github issue.

mstrelan made their first commit to this issue’s fork.

mstrelan opened merge request !25

mstrelan’s picture

mstrelan commented
Status: Active » Needs review

Generated the code with the help of an LLM.

Cspell doesn't like the word "unbans". We could ignore it, or perhaps it might prefer "un-bans".

The phpstan job is failing in the next minor job. We can fix that but it would start failing in the default job.

There is duplication with the drush command, but I don't know the best way forward.

mstrelan’s picture

mstrelan commented

Turns out we can reuse the same commands in drush by providing a drush.services.yml file. I've done this and tested both dr and drush are working.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

@mstrelan thanks! But it needs different files?
We can't simply reuse the existing https://git.drupalcode.org/project/ban/-/blob/1.1.x/src/Drush/Commands/B... ?

mstrelan’s picture

mstrelan commented

This replaces that. Drush is on the way out eventually, we don't want to have a Drush namespace.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Ah sorry now I got it!

IMHO let's add the "unban" term, I think it's clear and compact.

mstrelan’s picture

mstrelan commented

FWIW I just found this guide - https://www.drupal.org/docs/develop/drupal-apis/command-line-interface-c...

It suggests a different approach, but I think if this is working ok it is simpler. I'm probably out of time to work on this one now though, so if anyone else wants to get it over the line that would be great.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

I'm totally fine with your implementation and in my eyes it's good enough. Still very good to know about the suggestion from the link!

ressa’s picture

ressa
he/him
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Fantastic, thank you so much @mstrelan and @anybody, it works perfectly! I can ban an IP with dr ban:ban 1.3.4.5, list banned IP's with dr ban:list, remove all IP's with dr ban:flush, as well as unban. I have tested both with Drush and dr and both work perfectly.

PS. Perhaps we can add a note on the project page that only Ban 1.1 supports Drush commands, or release version 1.1 as stable soon? Currently you get Ban 1.0 with composer require drupal/ban, without Drush support.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: Unassigned » mstrelan

@mstrelan I'd be fine merging this and tagging a 1.1.0 release or do you see any blocker left?

#3582176: Add a performance test would be nice for confirmation regarding 1.0.0 -> 1.0.0 performance impacts, but I don't think it's a blocker?

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

I added the commands on the module page.

mstrelan’s picture

mstrelan commented

Go ahead!

anybody’s picture

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

Thanks!

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.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Sorry busy day, won't make it today to ship 1.1.0

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

1.1.0 released! 🎉

ressa’s picture

ressa
he/him
commented

Fantastic, thank you both for solving this so fast!

PS. The project page commands look not quite right ... maybe this could work better?

Ban dr Drupal CLI Commands

Available commands for the "ban" namespace:
  ban:ban        Bans an IP address.
  ban:flush      [ban:unban-all] Unbans all IP addresses.
  ban:list       Lists all banned IP addresses.
  ban:unban      Unbans an IP address.
  ban:unban-all  Unbans all IP addresses.
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Thanks @ressa sorry I fixed that - did that in a hurry. Fine now?

ressa’s picture

ressa
he/him
commented

That looks much better, and great idea with a compact version, thanks @anybody!

There is a detail, though: Some aliases do not exist for dr ...

$ dr ban-unban 1.3.4.8
  Command "ban-unban" is not defined.

Maybe just exclude aliases on the project page, since they are not crucial?

Ban Drush (drush) & Drupal CLI (dr) Commands

Available commands for the "ban" namespace:
  ban:ban        Bans an IP address.
  ban:flush      Unbans all IP addresses.
  ban:list       Lists all banned IP addresses.
  ban:unban      Unbans an IP address.
  ban:unban-all  Unbans all IP addresses.

(Drupal CLI support from >= 1.1.0 #3606943: Add dr support in Ban)

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Thanks @ressa good points!!

ressa’s picture

ressa
he/him
commented

Awesome, it looks very nice now, thanks!

Status: Fixed » Closed (fixed)

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