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

Improving exception msgs for database types #11764

Merged
merged 1 commit into from Feb 27, 2018

Conversation

burzum
Copy link
Contributor

@burzum burzum commented Feb 27, 2018

It's annoying that you can't see the type and make a guess which of the many values it might be.

throw new InvalidArgumentException('Cannot convert value to bool');
throw new InvalidArgumentException(sprintf(
'Cannot convert value of type `%s` to bool',
getTypeName($value)
Copy link
Member

Choose a reason for hiding this comment

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

Wouldnt it then also make sense to include the value itself directly? Or is that a bit too much then as debug info?

'Cannot convert value `%s` of type `%s` to bool'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to do that, but how could we archive this? It's just the base \InvalidArgumentException and doesn't really have a way to attach arbitrary data to it? Also does the error renderer has the capability of showing that value? Especially in this case of wrong type values, know the content of $value would make debugging the cause extremely helpful.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, maybe a bit out of scope - or even overkill - to use some Debugger::exportVar($x, 0).
Hopefully the stracktrace contains the caller arguments.

@dereuromark dereuromark added this to the 3.6.0 milestone Feb 27, 2018
@codecov-io
Copy link

codecov-io commented Feb 27, 2018

Codecov Report

Merging #11764 into 3.next will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             3.next   #11764      +/-   ##
============================================
+ Coverage     91.91%   91.92%   +<.01%     
  Complexity    13293    13293              
============================================
  Files           463      463              
  Lines         34164    34172       +8     
============================================
+ Hits          31403    31411       +8     
  Misses         2761     2761
Impacted Files Coverage Δ Complexity Δ
src/Database/Type/DecimalType.php 95.12% <100%> (+0.25%) 22 <0> (ø) ⬇️
src/Database/Type/IntegerType.php 100% <100%> (ø) 14 <0> (ø) ⬇️
src/Database/Type/BoolType.php 100% <100%> (ø) 16 <0> (ø) ⬇️
src/Database/Type/StringType.php 100% <100%> (ø) 13 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5c11df...c9c0eef. Read the comment docs.

@markstory markstory merged commit bb5e86f into 3.next Feb 27, 2018
@markstory
Copy link
Member

Nice improvement 👏

@markstory markstory deleted the 3.next-better-exception-msg branch February 27, 2018 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants