Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.So I'm tracing errors in the Aanmeldproces, I have "Display log errors on screen" set in the global configuration...
The first time I get this:
System generating error: acquirer (SE2000) (Directory)
iDEAL could not retrieve the list of issuing banks and will try again in ten minutes.
The list of available banks could not be retrieved.
The second time I just get this:
The list of available banks could not be retrieved.
...implying that the code was actually trying to retrieve the list of available banks. Which it wasn't: you are caching negative responses for 10 minutes, so if I just fixed my config... I'm confused now about why it is not working.
This could be more descriptive.
The code reads like you want to cache negative responses here. So I adjusted my patch to this. On the other hand... it can also be argued that with "Display log errors on screen" set, you can assume that people are testing stuff. And thus not cache the negative response in that case.
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | ideal_1590212_15.patch | 789 bytes | xano |
| #9 | ideal_1590212_01.patch | 789 bytes | xano |
| ideal-errormessage2.patch | 824 bytes | roderik |
Comments
Comment #1
xanoThis is by design in order to prevent sites from bombarding iDEAL servers with directory requests and logging errors if their configuration is incorrect, which may be caused by anything.
The message "The list of available banks could not be retrieved." is intended for the end user. The log message "iDEAL could not retrieve the list of issuing banks and will try again in ten minutes." is intended for the site administrator (that's why it is a log message).
Perhaps we can convert the "Display log errors on screen" to a "Development mode" that disables caching as well, but because that's a new feature, it won't make it into 7.x-2.x.
Comment #2
roderikYou're confirming what I thought.
So my patch-for-review is exactly in line with what you are saying here. It makes things clearer for the developer (who has explicitly turned these log messages on, which suggests he can use the extra feedback. Currently, the 'message meant for the user' is contradicting what the developer needs to know, in a setup procedure which is quite confusing enough already**.)
** not aimed at your module, but at iDEAL :)
Comment #3
xanoYour patch adds an extra message, but it does not solve the underlying problem of confusing messages. While part of the confusion is the consequence of showing both end-user and developer messages to a developer who tests the functionality as if he were an end user, we can, however, improve the log message by adding a "time left until next retry" sentence, somewhat like your patch, and a sentence about the cache.
Officially such a change should never be committed, because version 2 is already in string freeze. However, because this would only affect log messages which are intended for developers anyway, I think it's safe to squeeze this change in.
Comment #4
roderikSince part of this is subjective I feel like I can only say "go ahead then". Changing status to match your remarks.
Comment #5
xanoYou don't sound very convinced ;-)
Thing is this is a final release, so we cannot make any big changes. We can apply a subtle improvement, but we can not fundamentally change the way debug messages work. Also, part of the development process is to use the site like an end user will use it.
1) Not displaying the end user message when the option "Display log errors on screen" is enabled is a bad idea, which will only cause more confusion.
2) Displaying, but not logging, an extra debugging message only when the option "Display log errors on screen" is enabled, is confusing, because it does something different than what the option says it does.
3) We can, however, improve existing messages, because this doesn't change any behavior.
2 can be addressed in the next version by adding a full debugging mode. It is certainly a valuable addition and it could be implemented in the way you suggested with your patch. However, it's too big a change for an existing final branch.
If you have other feedback or idea, I'd be glad to hear it.
Comment #6
roderikThanks for the explanation. I agree with point 1. Not with point 2 but it's your module. Either way it's a message change -- which is not a huge thing.
Since the wording is probably dependent on opinion (also see my differing opinion on point 2), and changing the wording of a message is not a huge undertaking, I think it doesn't make sense
- to spend much more words on this
- for me to re-hash the patch.
So I propose to leave the re-wording & committing of the patch to you. That OK? :)
Comment #7
xanoA sidenote: as you know, Payment is supposed to replace most of iDEAL's current features. I just uploaded a patch that should address the same issues discussed here, but for Payment. In light of this issue, could you perhaps review #1675802: Add debugging mode for me?
Comment #8
roderikI'm interested in the whole Payment thing, but assigning to myself is all I can do at this moment...
Up to my neck in stuff (and that with DrupalCon just beginning). Feel free to shoot me a private message about 'deadlines' or anything.
Comment #9
xanoThe attached patch breaks the string freeze rule to improve the existing log message (and, when in debug mode, also the on-screen message) to tell site admins they can clear the cache to force iDEAL to load the issues from the remote server.
This solution keeps the messages and log entries consistent, which for me warrants the breaking string freeze. However, this discussion is old, and I may be missing part of the original problem.
Comment #10
roderikWell, yeah... Assuming admin does not overlook this message the first time (because it's printed only once in iDEAL v2), this is good. I think it can be committed and the issue closed.
I'm not changing status because I haven't actually tested this -- though I don't think this patch needs real testing.
Additionally, I just came across the similar message in v3, so --even though this issue does not cover v3-- I thought I'd mention things:
- the message in v3 -which is different- does not properly pass my English grammar parser (a.k.a. brain). You inserted an 'or' in between, but I suspect you wanted to change more.
- the message in v3 is invalid. It says 'will try again in 10 minutes" at a point where the response does not get cached, so in fact it will try again immediately.
No patch yet, because I'm not sure how you want things to be solved. Concrete questions:
1) Do you want to cache failed requests? (I assume yes, just like in v2, to 'not (potentially) bombard iDEAL with failing requests'. Which means you should probably
cache_set(cache_set($cid, array(), ...)before throwing an exception?2) If you do point #1, then after the initial error, there is no "message to the end user" (ref your comment #1 above). If a payment pane is rendered again. The user gets an empty list of banks in the dropdown, and no message is printed / nothing gets logged. This is different from v2.
Are you OK with this? (I think it's OK... The admin can see the original message in the logs, when tracing the problem.)
Comment #11
xanoGood catch! I'll fix these two bugs in 7.x-3.x.
When validating a payment, a directory request is executed. If that fails, the payment/payment method combination is invalidated, and the payment method will not show up in payment forms.
Comment #12
xanoSee #2048495: Failed directory request is not cached (7.x-3.x) for the cache failure.
Comment #13
xanoComment #14
roderikAh. Right.
So with #2048495: Failed directory request is not cached (7.x-3.x), #9 is RTBC. And obsolete, because the 2.x branch is obsolete as far as I understand.
Comment #15
xanoI replaced the comma by a full stop. Thanks!