Drupal 11 compatibility

Created an MR and attached a patch (same thing) with necessary changes for Upgrade Status to be happy.

You need the D10 compatibility patch as well, from #3289374

Combined D10 compatibility patch (from 3289374 📌 Automated Drupal 10 compatibility fixes Needs review ) with D11 patch from here. I'm not having any issues with this solution.

I merged the MR from the D10 update (6) into 7 and resolved the conflict.

This will allow everyone who contributed to them to maintain their commit authorship, as well as eliminate the need to install two conflicting patches.

nikolay shapovalov → made their first commit to this issue’s fork.

Added gitlab ci integration from 📌 Adopt Gitlab CI Active to MR 7, to test changes.

nikolay shapovalov → changed the visibility of the branch D11-fix to hidden.

MR 7 and MR 8 are identical, I hide MR 8 branch.
Let's focus on MR 7.

I spent some time trying to fix failed tests, but it looks like running tests on my local enviroment and on gitlab ci leads to different results. Not sure the reason. Myabe gitlab-ci.yml need some changes.

I found that at Giltab CI used IPv6

Your current IP address is ::1. If this is wrong, make sure you have the correct header configured in general settings.

Source you can find at artifacts Drupal_Tests_restrict_by_ip_Functional_RedirectTest-14.

nikolay shapovalov → changed the visibility of the branch 3479940-research-gitlab-tests to hidden.

Provide MR review.

Hide patch files, please use MR.

The d10 compatibility patch from https://www.drupal.org/project/restrict_by_ip/issues/3289374 📌 Automated Drupal 10 compatibility fixes Needs review has been merged in to 8.x-4.x-dev so i've merged that branch back in to here and resolved the conflicts. This has reduced the number of changes in this issue quite considerably. All but one of the comments on the MR from the last review related to the D10 changes. I've implemented property promotion as per the last review in the relevant file. Setting back to need review.

@nicrodgers hi, new maintainer here. I agree with your assessment on the comments on the MR - I am concerned about the failing phpunit tests though, particularly as this is a module people will be using for security purposes. We need to resolve those before merging any of this.

It may be that the tests are out of date in some fashion but that needs to be thoroughly researched.

No stress about the linting errors, those can be a follow-up.

@daraven maybe we should revert @nikolay shapovalov's commit in this issue that enables gitlab ci, and add it in a separate issue?

Whilst I agree having gitlab ci running automated tests would be a great position to be in, we don't have to do it as part of the Drupal 11 compatibility work. If we can get another pair of eyes testing the MR manually then we could get it RTBC'd and a Drupal 11 release shipped. We have been using the MR in production on various high traffic sites without issue for a while now.

@daraven - hi, btw :-) huge thanks for taking on maintainership of this module.

@nicrodgers if I was more familiar with the source code I might agree with you but it is a long time since I interacted with or even used this module. I put my hand up to maintain years ago and it just resolved - not a complaint, just explanation. I intend to take my custodial duties seriously.

The tests are failing which either means:

• the tests are not D11 compatible, or
• the ecosystem has changed enough that they're legitimately failing

It's good to know that you have been using this patch for a long time (and presumably so have others) but I'm not willing to cut a release based on that alone. I want people picking up the module or returning to it to have the most possible confidence in the new release.

I will be looking at the tests when I can manage to do so, I'm not putting this off awaiting someone else to fix them - though help is welcome :)

I just cut beta2 where I fixed/verified all tests as run by phpunit locally. It's certainly possible that chnages would be needed to get tests running on gitlab CI since there is a lot of work needed to mess with IPs and requests.

But at the least I can confirm that the tests are not bad.

Thanks very much @rocketeerbkw that's really good to know - may I ask which version of core you ran them against?

Tested locally on Drupal 11:

I can confirm the UI tests are passing locally, happy to let those be a follow-up. However it looks like the destination parameter is overriding the redirect to the access denied page which is something the tests seem to be there to ensure doesn't happen. Going to dig a little deeper into that.

All other tests are running fine so I don't think we need to mess with IPs and requests.

may I ask which version of core you ran them against?

Sorry that wasn't clear. I was verifying the Drupal 10 issue I merged in march and testing on HEAD, so I used 10.4.7.

It may be that the tests are out of date in some fashion but that needs to be thoroughly researched.

I Just wanted to mention here that the tests are working correctly outside of this issue and gitlab CI.

@rocketeerbkw thanks for the follow-up

I Just wanted to mention here that the tests are working correctly outside of this issue and gitlab CI. I haven't reviewed/tested this issue.

This is what I took your comment to mean, I really appreciate knowing that they're functioning correctly - my only concern is what may have changed in Drupal 11 that might have caused failures.

On that note I've tracked down the failure of the redirect test to the redirect function failing when the destination parameter is set to an invalid path - 'node' doesn't exist on minimal install in Drupal 11.

I confirmed the same behaviour on Drupal 10 (with a different destination param) so I've pushed a tiny change to use '/' instead on those two test and will open a follow-up issue to fix the existing bug.

The UITests are throwing ipv6 related errors on gitlab, we already have a follow-up for that: ✨ Not IPv6 compatible Needs work

I've rolled back the changes I made as they didn't help.

I've also rebased on the latest work on 8.x-1.x which I should have done in the beginning.

@rocketeerbkw's changes for beta2 solved the redirect issue which only leaves some UI tests. I'm going to commit this and move to managing follow-ups.

Release to come when ✨ Not IPv6 compatible Needs work goes in.