Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing driver exception annotations #4130

Merged

Conversation

morozov
Copy link
Member

@morozov morozov commented Jun 28, 2020

Q A
Type improvement
BC Break yes
  1. All driver-level methods that may throw an exception are documented with a @throws annotation.
  2. OCI8\Connection::getServerVersion() no longer throws an UnexpectedValueException.
  3. Mysqli\Statement::bindTypedParameters() and PDO\Statement::convertParamType() no longer throw an InvalidArgumentException.
  4. Driver-level methods are no longer allowed to throw any other exceptions that Driver\Exeption.

greg0ire
greg0ire previously approved these changes Jun 29, 2020
@morozov morozov force-pushed the missing-driver-exception-annotations branch from fd42f52 to 980c68c Compare June 29, 2020 20:37
@morozov morozov closed this Jun 29, 2020
@morozov morozov reopened this Jun 29, 2020
@morozov
Copy link
Member Author

morozov commented Jun 29, 2020

@SenseException could you review this, please? There was needed a rebase due to a conflict in UPGRADE.md.

@morozov
Copy link
Member Author

morozov commented Jul 1, 2020

Ping @greg0ire.

{
public static function new(int $parameter): self
{
return new self(sprintf('A non-stream resource passed as a LARGE_OBJECT parameter #%d', $parameter));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence feels a bit weird to me, I'd either use

Suggested change
return new self(sprintf('A non-stream resource passed as a LARGE_OBJECT parameter #%d', $parameter));
return new self(sprintf('A non-stream resource was passed as a LARGE_OBJECT parameter #%d', $parameter));

or

Suggested change
return new self(sprintf('A non-stream resource passed as a LARGE_OBJECT parameter #%d', $parameter));
return new self(sprintf('Non-stream resource passed as a LARGE_OBJECT parameter #%d', $parameter));

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we get rid of the past tense here and change it to something like: 'The resource passed as a LARGE_OBJECT parameter must be of type "stream"?' It will be more about how to fix the problem, not just the problem statement.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a side note, have you seen any articles/guides specifically on composing error messages? E.g. in navigation it seems acceptable to sacrifice the grammatical correctness of sentence in favor of brevity: do not lean on door). And many other computer programs I saw (especially in the past) would use this principle as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As another side note, we probably can get rid of this exception and let the underlying driver decide whether it can bind this resource to the statement or not. Then the message (if it's clear enough) may come directly from the driver (should be easier to google).

Copy link
Member

@greg0ire greg0ire Jul 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a side note, have you seen any articles/guides specifically on composing error messages? E.g. in navigation it seems acceptable to sacrifice the grammatical correctness of a sentence in favor of brevity: do not lean on door). And many other computer programs I saw (especially in the past) would use this principle as well.

I didn't but it's an interesting topic for sure!

Copy link
Member Author

@morozov morozov Jul 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just googled it for myself and will leave it here:

Signs often have reduced grammar, as do newspaper headlines, captions of photographs, and so on. They may omit articles and auxiliary verbs, for example. We're so used to this that it might actually sound strange to use a complete sentence in such situations.

This is by no means a justification for my illiteracy.

Copy link
Member

@greg0ire greg0ire Jul 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha I stumbled upon this blog post that refers to us while looking this up: http://rosstuck.com/formatting-exception-messages

This one is quite interesting: https://softwareengineering.stackexchange.com/questions/29433/how-to-write-a-good-exception-message, but more about the contents than the form.

This one advocates against brevity: http://amortizedcost.net/writing-good-exception-message/

IMO there might have been a point in brevity in the past, when memory was an issue, but nowadays we might want to revisit that thought and start using multiline exception messages (php 7.3 makes heredoc and nowdoc nicer BTW) , which detail the context, the problem and solutions as suggested in the second link.

@morozov morozov force-pushed the missing-driver-exception-annotations branch from 980c68c to 4f7c86f Compare July 1, 2020 19:02
@morozov morozov merged commit 5bca2b7 into doctrine:3.0.x Jul 1, 2020
@morozov morozov deleted the missing-driver-exception-annotations branch July 1, 2020 19:15
@morozov morozov self-assigned this Jul 1, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants