- Issue created by @Grevil
- First commit to issue fork.
- Merge request !26Changed logging from critical to info, as there's nothing we can do. → (Merged) created by Anybody
- Assigned to Grevil
- Status changed to Needs review
8 months ago 12:45pm 18 April 2024 - 🇩🇪Germany Anybody Porta Westfalica
Here we go. But it might make sense to change the texts a bit so that the reader (admin) better understands the context and meanings?
And should we add an option to turn the noise off perhaps? ;)
Or the other way around: "Debug mode" with a matching desciption?What do you think? (Could also be done in a follow-up to get this done.)
- Status changed to Needs work
8 months ago 1:25pm 18 April 2024 - 🇩🇪Germany Grevil
Not quite what I envisioned! Since we now still have the issue of duplicate logging. I'll show you what I envisioned here.
- Issue was unassigned.
- Status changed to Needs review
8 months ago 1:50pm 18 April 2024 - 🇩🇪Germany Grevil
And should we add an option to turn the noise off perhaps? ;)
Or the other way around: "Debug mode" with a matching description?We could think of such a feature, I'd vote for having an option to turn off the noise. Not really a fan of the other way around, since I see the current error logging as necessary and something that you should deliberately have to turn off. But let's do that in another follow-up issue!
- Status changed to Needs work
8 months ago 2:08pm 18 April 2024 - Assigned to jurgenhaas
- 🇩🇪Germany Grevil
Hm, interesting. Our solutions array always consists of 4 parts, instead of the expected 3 parts. I already reinstalled the module and it still consists of 4 parts:
I mean our original code:
[$signature, $puzzle, $solutions] = $solutionArray
would simply ignore the 4th array entry, but I am still wondering what it is used for...@jurgenhaas, any idea?
- Assigned to Grevil
- 🇩🇪Germany Anybody Porta Westfalica
We could think of such a feature, I'd vote for having an option to turn off the noise. Not really a fan of the other way around, since I see the current error logging as necessary and something that you should deliberately have to turn off. But let's do that in another follow-up issue!
Correct, if these are errors / notices the admin can do something about. If not, it should be off by default. And that was what I expected here.
- Assigned to jurgenhaas
- Status changed to Needs review
8 months ago 2:28pm 18 April 2024 - 🇩🇪Germany Grevil
@Anybody ok, I will create another follow-up issue for that. Let's keep this issue assigned to @jurgenhaas for the problem mentioned in #9 📌 Adjust the logging for the local endpoint implementation Active
- 🇩🇪Germany jurgenhaas Gottmadingen
I mean our original code: [$signature, $puzzle, $solutions] = $solutionArray would simply ignore the 4th array entry, but I am still wondering what it is used for...
That verify code comes from the friendly captcha folks, it's a clone of their own server. So, this is how they implemented that. I guess, that 4th argument is just relevant for that verify process.
Here is the link to the original code: https://github.com/FriendlyCaptcha/friendly-lite-server/blob/e93be67590a...
And indeed, they don't use that 4th one anywhere.
I'd suggest that we check for at least 3 parts in the array, not for exactly 3 parts.
- Assigned to Anybody
- 🇩🇪Germany Grevil
@jurgenhaas thanks for clearing this up! 👍
Originally, I thought we should still check, that the array consists of exactly 4 parts, because in cases where it consists of only 3 parts the order of the array entries might be incorrect, leading to incorrect variable declarations, which might lead to other errors.
But one could think, that the friendlyCaptcha folks thought of that.
But since the original error with the 2 parameters instead of 4 puts me on the fence... Final thoughts @Anybody? Check for exactly 4 parts or at least 3? - 🇩🇪Germany Anybody Porta Westfalica
I think we should check for the three required parameters ("at least three"), but add a comment about the 4th parameter:
"$diagnostics" which is unused. Perhaps linked to this discussion? - 🇩🇪Germany Grevil
Alright! Adjusted the code accordingly. :)
Also used @ instead of % for the placeholder replacements, as the tags are being escaped on the recent log message:
- Issue was unassigned.
- 🇩🇪Germany Anybody Porta Westfalica
@Grevil: Nice!
I left some comments. These are just meant to be placed as prefix in front of the message so it's easier for the admin to see what it's about!
Also I'm sure my comment form #10 applies. The admin can't do anything about it, so the debug mode should be opt-in!
- 🇩🇪Germany Grevil
Captcha validation failed! ID: "solution_malformed" Message: "Malformed solution: Expected at least 3 parts, got 4.".
LGTM now.
- 🇩🇪Germany Grevil
Here is the follow-up issue concerning #10: 📌 Create an option to turn off any logging from this module Active .
- 🇩🇪Germany Anybody Porta Westfalica
Nice! RTBC!
If @jurgenhaas has comments on the commit, let's solve them in a follow-up.
- Status changed to Fixed
8 months ago 8:50am 19 April 2024 - 🇩🇪Germany Grevil
Thanks! Make sure, to always set the MR label manually. It doesn't get generated automatically according to the issue name / id anymore.
Automatically closed - issue fixed for 2 weeks with no activity.