GH-8458: DateInterval::createFromDateString does not throw if non-relative items are present

[derickr]

No description provided.

[iluuu1994]

Looks like a nice improvement 👍

[cmb69]

I'm not sure if we should throw here; if the format is unknown or bad, we raise E_WARNING, so maybe we should do it here as well. But if we throw, we should consider to throw a ValueError (zend_value_error()).

[derickr]

I would argue that we should throw in the other case as well really, as you're not getting a usable object. But I now realise that this is both a procedural API (date_interval_create_from_string()) and an OO one (DateInterval::createFromString). I reckon we should warn for the procedural API, but throw in the OO variant.
What do you think?

[cmb69]

In a perfect world, I would throw a ValueError in all these cases (possibly not for the procedural APIs). But I'm somewhat concerned about the BC break (as I understand it, so far parts of the strings may have been ignored, but there still may have been a valid/usable DateInterval).

[Girgias]

IIRC when I was doing the large chunk of warning to error promotion the general guideline which we had with @nikic was that if it is a programming error or relatively easy to check it should throw an error, otherwise it's kept as a warning. Mainly as the string my be provided from a user and not within the control of the dev.

Although, I do think ideally if this was a new function/method it should be a ValueError but like @cmb69 said I wonder about the potential BC, but then I have a PR which fixes a bug which also goes from no warning to TypeError soooo

Aside, could you add a test for the procedural version?

[derickr]

I've just pushed an update to this, and it will now always throw a warning, and not an exception. Although you can argue that there is a BC break, because FALSE is now returned if the date string is broken, at least it is now not silently ignoring a programming mistake.

I also added a test case for the procedural variant.

[cmb69]

I think this BC break is acceptable for a new minor version. Others might disagree, though.

(consider to change the NEWS entry; the method still doesn't throw in this case)