Problem/Motivation

Both #3392147: Add an allowed IP list to the Ban module and #3553606: Add drush unban commands introduced breaking changes. Circumvent them instead.

Steps to reproduce

Proposed resolution

Throw a deprecation notice and use the \Drupal::service() method, wherever needed.

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork ban-3575604

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

grevil created an issue. See original summary.

grevil opened merge request !11

grevil’s picture

grevil commented

Ok, I adjusted the constructor call, but I am unsure, how to revert the "unbanAllIps" from the interface.

We could simply remove it from the interface and keep it on the manager, but I am not really a big fan of that either... but I guess that is the only way.

grevil’s picture

grevil commented
Assigned: grevil » Unassigned
Status: Active » Needs review

Please review!

angel_devoeted’s picture

angel_devoeted commented

Ban unit tests ran successfully.
The drush ban:flush also runs without errors.

In BanMiddleware::__construct(), the $config_factory argument is nullable but still required (no default value), so the deprecation path for calling it without the argument is not actually reachable.

unbanAllIps() has been removed from BanIpManagerInterface, but BanCommands::unbanAll() still calls $this->banIpManager->unbanAllIps().

anybody’s picture

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

I think it makes sense to get @mstrelan's feedback here before we proceed. We should then also discuss creating a 2.x branch now or later with the interface additions?

mstrelan’s picture

mstrelan commented
Status: Needs review » Needs work

We now have an issue in BanCommands where it's calling $this->banIpManager->unbanAllIps(), but since we are typing to the interface it's possible the actual implementation does not have that function. So either we need to check if the method exists, or put it back in the interface and roll out 2.x. FWIW I'm pretty sure phpstan level 8 would have detected this before it was rolled back to level 0.

grevil’s picture

grevil commented
Assigned: mstrelan » Unassigned
Status: Needs work » Needs review

Let's keep it like this and publish it in 1.1.x.

I don't think anyone implements the BanIpManagerInterface without extending the BanIpManager. Of course it is technically not best practice to introduce breaking changes in a minor release, but I think we can make an exception here.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Thanks @grevil, I won't set this RTBC, instead @mstrelan should please decide what should finally get reverted and how it should get resolved now. Shouldn't be our decision and these are all valid points. 2.x feels a bit early to me. I'm fine with any final decision @mstrelan makes on the next steps and implementation.

If it's fine like this, please RTBC.

mstrelan’s picture

mstrelan commented

I don't think anyone implements the BanIpManagerInterface without extending the BanIpManager. Of course it is technically not best practice to introduce breaking changes in a minor release, but I think we can make an exception here.

Yeah, that's probably fine.

2.x feels a bit early to me.

Yes, while people are transitioning from core over the next few months they should be able to run composer require drupal/ban and not have breaking changes. We can open 2.x if we want to, but let's not release anything there yet.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

@mstrelan @grevil I agree with that. Let's get things done here?

@mstrelan maybe create 2.x once all relevant issues that don't require breaking changes are resolved? Otherwise, it will just be additional work.

grevil’s picture

grevil commented
Status: Needs review » Needs work

@anybody the main problem is, that we already created an 1.1.x branch with no release. And if that branch now contains no (real) breaking changes, I'd suggest using that.

We can introduce breaking changes in 2.x since it is a major release, so I don't think it makes sense to create a major release while we already have a minor release branch setup.

But I am generally a bigger fan of major release branches, so we could just skip and ignore 1.1.x entirely. It has a dev release, so we can not completely kill it unfortunately.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Okay let's just resolve things and stick to 1.1.x-dev for now.

grevil’s picture

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

  • grevil committed aede998f on 1.1.x 
    feat: #3575604 Remove introduced breaking changes in 1.1.x
grevil’s picture

grevil 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.

Status: Fixed » Closed (fixed)

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