Add InvalidArgumentException as throwable to ValueFormatter documenation

[AlaaSarhan]

Bug: https://phabricator.wikimedia.org/T207479

[AlaaSarhan]

I can also move replacing hhvm with php 7.2/7.3 to a separate PR if you prefer

[addshore]

I can also move replacing hhvm with php 7.2/7.3 to a separate PR if you prefer

Its probably fine in this PR, but we might not want to remove hhvm as we still need to be compatible with it.

It looks like the error was just:
Fatal error: Uncaught exception 'InvalidArgumentException' with message 'array_pop() expects parameter 1 by reference, but the call was not annotated with '&'' in phar://composer.phar/bin/../src/../src/Composer/Util/Silencer.php:51
So we might be able to just fix that and re add hhvm to travis

[AlaaSarhan]

yes we can also fix it if we need to support hhvm

[manicki]

until WMF infrastructure switches to PHP 7.x HHVM is what this code is being run on, so if we could get Travis CI set up fixed without doing anything nasty, this would of course be sane :)

[AlaaSarhan]

@manicki yup we can fix HHVM version on travis to the latest that support PHP (3.x). What version do we run this code on on WMF?

[JeroenDeDauw]

From the point of view of the user of the interface, why would there be two exception types, rather than just FormattingException? I suspect this makes the interface on average less fitting.

Then again if there are many implementations violating the contract of the interface the most pragmatic thing might indeed be to update the interface. Especially since this is a very dubious interface to begin with. I've said it before over the years: I don't think widespread usage of this interface in Wikibase is a good thing and that most of the time it is better to work with context specific interfaces.

Strictly speaking this is a breaking change and requires a major release. A major release would cause a lot of work though, so if this is done, pretending it is not a breaking change might be the best way forward.

[AlaaSarhan]

@JeroenDeDauw yes I had a similar train of thoughts while changing it. I guess using exception in general in interfaces is always a bad idea anyway.

I'm not sure this is a breaking change strictly speaking. As far as I can tell, this won't break any usage of the library (except maybe breaking some weird test case that asserts this method won't throw any other exception).

[JeroenDeDauw]

I guess using exception in general in interfaces is always a bad idea anyway.

Not heard that before. Why would it be? Got any links on that?

I'm not sure this is a breaking change strictly speaking. As far as I can tell, this won't break any usage of the library (except maybe breaking some weird test case that asserts this method won't throw any other exception).

I also expect none of the code that uses this interface in Wikibase to break. This is a public library so someones code might break. Probably not though.

I'm quite sure this is strictly speaking a breaking change, as it widens the "return type" of the method, which existing users might not be able to deal with.

+1 from my side

[AlaaSarhan]

I guess using exception in general in interfaces is always a bad idea anyway.

Not heard that before. Why would it be? Got any links on that?

Nope not yet actually.. I should've maybe added 'imo' there, but the 'I guess' suggests it.

I am also contradicting myself if I go back to the original comment. This is a utility library, I realized, so using exceptions in it shouldn't contradict with my following guideline.

But here's in short the opinion I have on it. Exceptions are the least meaningful piece of specification, esp. in abstractions and more so in languages where you cannot enforce them, like here in php. A good practice I try to follow, based on mere experience (so I might still haven't learned the right lesson yet) is to keep exceptions out of my design and push them as far as possible down to implementation details where nothing else can be done (most common case is passing erroneous arguments to constructors where no data type was used to constrain the ranges/formats). Here's why:

• exceptions are a separate thread of execution, originally designed to handle system entirely unexpected and not-design-for 'crashes'. These days we can add also language-level or even framework-level crashes. They were not designed for wrong usage of Domain/Business logic.
• expected errors in business logic should be part of the design, and handled as part of that business logic all the way up.

That's just a general guideline though.. I myself take it with a bag of salt, but it's an interesting topic maybe I write about it soon, or better we can have a discussion about it and write that down afterwards. Should be fun :)

[manicki]

I can subscribe to concerns about the interface use, and about throwing different kind of exceptions, but indeed it seems to be the sane thing to do overall, when thinking pragmatically. Hence merging the change.

Re breaking change: as this library is still in 0.x range, we could still go up to 0.3.0 (per semver FAQ "The simplest thing to do is start your initial development release at 0.1.0 and then increment the minor version for each subsequent release."), should bumping up to 1.0.0 be a concern.

[JeroenDeDauw]

@manicki you are kinda missing the point. 0.x range or not, it is still a breaking change. Updating to 0.3 causes just as much work as switching to 1.0. https://www.entropywins.wtf/blog/2018/08/30/base-libraries-should-be-stable/

If a switch is made, then it should be to 1.0, since it makes no sense this package is still in the 0.x range.

[JeroenDeDauw]

I noticed you did not release this yet and also noticed that master already contains breaking changes so can only be released as 1.0. This means to use the change made in this PR you'll either need to backport it onto 0.2.5 and release it as 0.2.6, or to release 1.0.

I just had a look at what needs doing for 1.0 and this is the only thing I spotted: #45. If the change in this PR is not super urgent, it probably makes sense to just have it in 1.0 and not spend extra time on backport. Having everything updated to use 1.0 will of course take a bit of time since libraries depending on this one will need to have their own releases made.