Skip to content

Conversation

@jdecool
Copy link
Contributor

@jdecool jdecool commented May 19, 2022

Currently the Safe\json_ functions throws a Safe\Exceptions\JsonException which is not related to the standard PHP JsonException.

So this PR add intheritance between Safe\Exceptions\JsonException and \JsonException to solve this issue.

@codecov-commenter
Copy link

Codecov Report

Merging #357 (9996ecd) into master (b5309a3) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #357   +/-   ##
=========================================
  Coverage     49.55%   49.55%           
  Complexity      313      313           
=========================================
  Files            16       16           
  Lines           791      791           
=========================================
  Hits            392      392           
  Misses          399      399           

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 b5309a3...9996ecd. Read the comment docs.

@Kharhamel
Copy link
Collaborator

Are you sure this doesn't break anything? I got some really bad suprises with some recent PRs

@jdecool
Copy link
Contributor Author

jdecool commented May 24, 2022

Are you sure this doesn't break anything? I got some really bad suprises with some recent PRs

I don't think so.

Previously it throws an \Exception. Now it will throw an \JsonException which inherits from the standard \Exception (see https://www.php.net/manual/en/class.jsonexception).

So it shouldn't have any BCB.

@Kharhamel
Copy link
Collaborator

That's what I though too, but then stuff like #253. I am still going to merge this because I don't think there is a way to anticipate this kind of issues anyway

@Kharhamel Kharhamel merged commit 5570fbf into thecodingmachine:master May 25, 2022
@jdecool jdecool deleted the json-exception branch May 25, 2022 13:52
@jdecool
Copy link
Contributor Author

jdecool commented May 25, 2022

Thanks @Kharhamel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants