Route allowed without specified parameter is blocked when a parameter is passed in the url

if (!$params || !$query_string) { is exactly the same as if (!$params || ($params && !$query_string)) {, so that would be shorter if it works.

Please review the merge request.

Marking this issue as Major, since it would prevent most potential uses of this module altogether.

Thanks! I approve, but I think I can't do it on git.drupalcode.org. Sorry :(

A maintainer has to do that part.

Working on creating tests.

The requestAffected() function didn't have any tests for it at all! I added test coverage for the entire function, not just for the changed functionality.

It's a good thing I spent time last week reading about how phpspec/prophecy works, because the module's tests use it a lot.

Adding as a related issue, since I fixed its failing phpunit test here.

Shorter title.

Still waiting on review.

I just bumped into this issue as well. At least for me, leaving the params option empty should mean that we don't care about matching the query string.

So instead of

if (!$params && !$query_string) {
...
}

We could just have

if (empty($params)) {
...
}

What do you think?

Did you try the patch from the MR? The MR uses if (!$params || !$query_string) {, which should also work.

Yeah, the MR does work for my case. I wonder about the logic still, though. If a user specifies params, they probably want to make sure the request params match right?

!$params || !$query_string would work for cases where no params were specified, but also where params were specified but no query string is present.

What do you think? I could see the argument either way, really.

Same. I don't have that use case personally, so I don't really know either way, to know which is better.

I think that argument makes sense, though. The tests will also need to be updated. Specifically this part, since we would be changing it to not be affected if there were no query params given but the route restriction has query params specified:

    $this->request->getMethod()->willReturn('GET');
    $request_affected = $this->restrictIpService->requestAffected();
    $this->assertEquals(TRUE, $request_affected, 'Request affected for method match and empty params.');

Review the changes now. I added more tests for more use cases.