Subject should be optional in Logger\Logger::logException()

[mxr576]

https://github.com/Pronovix/password_enhancements/pull/1#discussion_r299030627

[ycecube]

The reason why it was introduced to have a clear picture about the errors in the dblog, making the subject optional would kill this intention, so I would keep it required.

[mxr576]

Yes I know, I suggested but, if the thrown exception provides enough details about the root of the error then the message could only repeat what the exception already exposed.

You can see some examples for actually useful exceptions that tells what went wrong in this PR review commit https://github.com/Pronovix/devportal-drupal-module/pull/77#discussion_r303773771 and in the repo: https://github.com/Pronovix/devportal-drupal-module/tree/8.x-2.x/modules/api_reference/src/Exception https://github.com/Pronovix/devportal-drupal-module/blob/8.x-2.x/modules/api_reference/src/Plugin/Reference/OpenApi.php#L133-L181

[ycecube]

That's why I didn't wanted to add subject, because the exception originally should provide enough information.

I think we should either have it and make it mandatory or remove the subject. If it would be optional people probably wouldn't bother providing an error message, also because it's not always clear what throws an exception in Drupal and what kind of an exception (badly documented functions or the exception is simply hidden in some way so the IDE can't figure it out).

IOW have it or not, but I wouldn't make it optional, just for the sake of consistency.

[mxr576]

IOW have it or not, but I wouldn't make it optional, just for the sake of consistency.

I would keep it optional and suggested to have it configurable earlier to cover when we do not know what kind of exception is thrown - whether it provides useful information or not - but we do want to make our life more comfortable if we have to figure out what went wrong from the logs. With the optional subject, we can provide additional context and human-readable explanation that what could go wrong and where and that information could be seen from the excerpt from the log message.

Having something optional does not mean that it is always useful and manual code reviews should discover cases when the subject should be provided because the caught \Exception may not provide enough context.

[ycecube]

Okay then, the question now is that we should do this BC change now or sometime later?

[mxr576]

There is no BC guaranteed until beta1 and we only have dev :) I have just added this tag to let us know this must be handled before beta1.