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

Introduce subtypes of InvalidArgumentException #809

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

renanbr
Copy link
Contributor

@renanbr renanbr commented Mar 21, 2025

This PR introduces subtypes of InvalidArgumentException.

This improves the log analysis and allow users to rely on type instead of exception message when handling specific errors.

@frederikbosch
Copy link
Member

I am pretty alone here managing this package, but personally I think this is a good PR. There is no BC-break and I can see why the specific exceptions add value.

@ruudk Since you have actively helped getting PHP8.4 support in, would you be willing to provide some feedback?

Copy link
Contributor

@ruudk ruudk left a comment

Choose a reason for hiding this comment

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

Looks like a good change to me, except for the typo in ModuleByZeroException class name. From what I can tell, this won't have a BC impact. The newly more specific exceptions that are thrown in Money.php still extend from \InvalidArgumentException.

@renanbr renanbr requested a review from UlrichEckhardt March 28, 2025 09:46
@frederikbosch
Copy link
Member

There is a PHPStan error. If someone is able to see what is causing this, and resolve it, I can merge it. Otherwise I will have a look myself, but I don't know exactly when I have time for that.

@renanbr
Copy link
Contributor Author

renanbr commented Mar 28, 2025

@frederikbosch #810 may solve the PHPStan error

@frederikbosch
Copy link
Member

Now this one can be rebased against current master and then merged.

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.

4 participants