Skip to content

Add flexibility to report problems in different ways - #22

Merged
webmozart merged 1 commit into
webmozarts:masterfrom
keradus:error
Oct 17, 2016
Merged

Add flexibility to report problems in different ways#22
webmozart merged 1 commit into
webmozarts:masterfrom
keradus:error

Conversation

@keradus

@keradus keradus commented Oct 12, 2016

Copy link
Copy Markdown
Contributor

Hi !

Further extending of #19 to allow report a problem in different ways via extending the class, while default behaviour is not changed.

Use case - I have a library used with a lot of messy, invalid input from it's users that needs to be fixed by the users of library.
When I do the assertions that raising the exceptions - it will crash their app. So far even if their input is not perfect, it works somehow (not in the best way, but at least some functionality is working due to autoconverting).

The goal is to be strict and raise an exception when user of lib will pass invalid input. But as a middle step I want to make extra release which will just warn him (like triggering a warning instead of breaking the flow with exception).
Currently one can't override the library to do anything else than raising an exception. With this change it would become possible.

What do you think @webmozart ?

@webmozart

Copy link
Copy Markdown
Member

Looks fine to me. What do you think @Nyholm?

@Nyholm

Nyholm commented Oct 14, 2016

Copy link
Copy Markdown
Contributor

Thank you for this PR. I really like the idea but Im not sure about the implementation.

Why do we need a reportProblem function?

I suggest we rewrite the CreateInvalidArgumentException to:

protected static function reportInvalidArgument($message)
{
    throw new InvalidArgumentException($message);
}

That would also solve your problem, right?


BTW this is a blocker for tagging 1.2. We really have to do this before or else this would be a BC break.

@keradus

keradus commented Oct 14, 2016

Copy link
Copy Markdown
Contributor Author

Your proposal was my initial solution, but I realized it would break existing interface, so it would be a BC breaker.
Now indeed I see that create... method is not yet released, so indeed we could change it ;)

@Nyholm

Nyholm commented Oct 14, 2016

Copy link
Copy Markdown
Contributor

Awesome

@keradus

keradus commented Oct 14, 2016

Copy link
Copy Markdown
Contributor Author

updated

@Nyholm

Nyholm commented Oct 14, 2016

Copy link
Copy Markdown
Contributor

Great. Im 👍

What does @JustBlackBird think? You introduced the custom error reporting.

@JustBlackBird

Copy link
Copy Markdown
Contributor

Looks good.

At the same time personally I don't think that many people will use reportInvalidArgument method for anything but throwing exception ('cause it's most obvious behavior to throw an exception when assertion is failed) 😄

@keradus

keradus commented Oct 15, 2016

Copy link
Copy Markdown
Contributor Author

So, everyone is happy about this PR. What can I do to make it happen ?

@Nyholm

Nyholm commented Oct 15, 2016

Copy link
Copy Markdown
Contributor

Yes. We are all good. I know that @webmozart will merge this one and #24 as soon as he gets a minute over for open source work.

@webmozart

Copy link
Copy Markdown
Member

Thanks all!

@webmozart
webmozart merged commit e850ef7 into webmozarts:master Oct 17, 2016
@keradus
keradus deleted the error branch October 17, 2016 08:07
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