- Issue created by @franckylfs
- Status changed to Needs work
over 1 year ago 10:55pm 7 December 2023 if (!$params || !$query_string) {
is exactly the same asif (!$params || ($params && !$query_string)) {
, so that would be shorter if it works.- Merge request !4Allow if no params in URL or none in configuration. → (Open) created by solideogloria
- Status changed to Needs review
about 1 year ago 8:59pm 14 May 2024 Marking this issue as Major, since it would prevent most potential uses of this module altogether.
- 🇨🇦Canada franckylfs Montreal
Thanks! I approve, but I think I can't do it on git.drupalcode.org. Sorry :(
- Status changed to RTBC
about 1 year ago 1:50pm 15 May 2024 - Assigned to solideogloria
- Status changed to Needs work
about 1 year ago 1:51pm 15 May 2024 - Status changed to Needs review
about 1 year ago 2:32pm 15 May 2024 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.
- Issue was unassigned.
- 🇺🇸United States tyfoo
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.- 🇺🇸United States tyfoo
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.');
The changes here will not apply once 🐛 2.0.x not compatible with Drupal 11 Active is merged. It will need to be rerolled.
However, that issue is for D11 compatibility, so please review and test it, and then we can reroll this issue.
Please look at and review 🐛 2.0.x not compatible with Drupal 11 Active , which contains the README.md that was added to 1.x