Enh: Attempt to show better error messages when DB-Connection is invalid #6512
Conversation
PR Summary
It's worth noting that there are other minor changes in the codebase, mostly updating the usage of Also, it's confirmed that a few files only received timestamp updates or comments/whitespace revisions and one file has not been amended at all. |
martin-rueegg
force-pushed
the
enh/db-connection-error
branch
3 times, most recently
from
0a05d3e
to
728e13f
Compare
martin-rueegg
force-pushed
the
enh/db-connection-error
branch
from
728e13f
to
68c283b
Compare
There was a problem hiding this comment.
@martin-rueegg Thanks, looks good for me. If this is not only used on console usage, we need to ensure that no database creds are leaked.
I see, great point, thank you! Because the password would be included in the connection string? I guess, we could leave the output of the connection string to the console and otherwise print an error to the log. Would that do? |
|
@martin-rueegg Currently it is only used in the console context anyway, right? If so, we can merge it. |
martin-rueegg
force-pushed
the
enh/db-connection-error
branch
from
68c283b
to
56e9cf4
Compare
|
@luke- nope, it is also shown in web context. But I have entirely removed the DSN in web context. So the only way where the DSN would be shown on the web is while running migrations, as this is run as a console task from the webserver, right? But a migration is only run by humhub admins? It could still be a problem as the humhub admin and the web server admin aren't necessarily the same people. Otherwise I completely remove the DSN and write the message to a temp-file and only display the path of that temp file to the user (as it makes no sense sending anything to the log which might try to access the db as well). I mean it's a rare case where this scenario happens, eg. when moving the installation from one server to another, when changing the DB server or things like that. |
|
@martin-rueegg Ah sorry, I missed the In the web context, I would only output details when the software is running in debug mode. (Currently by default). Otherwise, there should be no detailed output at all in web. In the migration (web)/console, there is no problem if there are some more detailed information. Maybe we can search and replace the DB dassword with ***? |
Actually, the index.php change wasn't there, but it was in the bootstrapping phase when checking the db-connection. |
@luke- I've did my best: https://regex101.com/r/IRO3rG/2 Console with debugConsole without debugweb with debugInvalid database configurationDatabase not found. The following connection string was used: Technical informationWeb without debugInvalid database configurationDatabase not found. |
martin-rueegg
force-pushed
the
enh/db-connection-error
branch
2 times, most recently
from
578211a
to
cbeaf7d
Compare
There was a problem hiding this comment.
@martin-rueegg Looks very good. Thank you!
I've been thinking about introducing a humhub\helpers namespace for a while and this could be worth to be added into DatabaseHelper or ErrorHelper there for example.
Should we do this with this PR?
I can also do this afterwards...
|
In my point of view, However, I'm happy start the thing. I would, however, not touch any other of the
If we create a It could also make sense to have the How do you want it to be or shall I make a suggestion along these lines? |
Good point + also the hint regarding the GIT tracking.
Migration from
I would prefer: we finish this PR. One more change should be to move the new helper method to its own file in the Further improvements e.g. the suggested |
martin-rueegg
force-pushed
the
enh/db-connection-error
branch
from
cbeaf7d
to
582a2ca
Compare
|
@luke- like so? |
Yes, it is a underestimated or unknown limitation of GIT, that it does not track actual file moves. All it does, when committing (at the end, be it a squashed commit) see, if any newly created file resembles any deleted file. That means, that if you move a file and change it at the same time too much, GIT will commit a delete and create, rather than a move. An example of this is the move of Hence, best thing is:
|
|
@martin-rueegg Especially point 6 is important, because I usually always squash merge PRs. :-) |
What kind of change does this PR introduce?
Does this PR introduce a breaking change?
If yes, please describe the impact and migration path for existing applications:
The PR fulfills these requirements:
developbranch, not themasterbranch if no hotfixOther information:
When the connection information is wrong, or the database server name resolution fails, there are misleading error messages as there is actually no error handling of that situation so far.
This PR is a poor man's attempt to give the user some sort of helpful information. Yes, it does not look pretty and styled, but that's partly because the theming tries to access the db itself...
Particularly un-informative is the error when you try to run migrations with an invalid connection configuration.