null-friendly methods

[gnutix]

In our application, we often times have to create Brick objects (LocalDate, Duration, TimeZone, etc) with code like this :

null !== $var ? Duration::parse($var) : null

It would be nice if methods like parse-rs would allow null in input, and directly return it if it is null indeed. It would go along with a PHPDoc stating @return ($input is null ? null : self), so that static analysers can tell which result it will be. And that way, no more bloated ternaries at call-site.

WDYT ? I'd be willing to work on a pull request in the upcoming two weeks, if that's something you would accept in the library. Let me know!

[gnutix]

In the same vein, we have this DurationHelper class that allows us to have plus and minus null-friendly operations :

final class DurationHelper
{
    /**
     * @return ($a is null ? ($b is null ? null : Duration) : Duration)
     */
    public static function plus(?Duration $a, ?Duration $b): ?Duration
    {
        if (null === $a && null === $b) {
            return null;
        }
        if (null === $a) {
            return $b;
        }
        if (null === $b) {
            return $a;
        }

        return $a->plus($b);
    }

    /**
     * @return ($a is null ? ($b is null ? null : Duration) : Duration)
     */
    public static function minus(?Duration $a, ?Duration $b): ?Duration
    {
        return self::plus($a, $b?->negated());
    }
}

It would be much nicer if that would be integrated into Duration::plus() and minus() (and maybe others) directly.

[gnutix]

Still in the same vein, we use $date?->jsonSerialize() a lot when a date might be null, but if it's not, we want its ISO-8601 representation. Which is quite ugly, because it's not at all related to JSON serialization. But yet I find it less ugly than $date?->__toString() ( 🤮 ).

So I'm wondering if we could we add a public function iso(): string method that would return the same value as __toString() ? $date?->iso() looks nice.

Or at least a toString method without the magic method's __.

Related : this could have been fixed by https://wiki.php.net/rfc/nullable-casting using (?string) $date, but seems like the RFC hit a dead-end.

[BenMorel]

Hi, while I understand the use case, I don't think this is something that belongs to this library. A thin helper class is probably fine for this!

Regarding __toString(), it may look a bit ugly to call with the null-safe operator, but I'm a bit reluctant to add yet another method that does the same thing :/

[gnutix]

I dislike having static helper classes everywhere for such simple things that could easily be integrated in libraries and improve its developer experience. Moreover, it means adding one more way of doing things, which means there'll be consistency issues in a big enough team/project.

Why do you decided not to consider such simple changes ? Why don't they belong in the library in your opinion ?

[tigitz]

I like the fact that the library is warning me at runtime that the value I'm passing is not parseable.

What I suggest is using the same pattern as Enum creation and implementing a tryParse the same way Enum has tryFrom

I think it would be the sensible thing to do.

[gnutix]

implementing a tryParse the same way Enum has tryFrom

That could be an approach indeed.

I must admit I tend to forget that not everybody use and rely on a static analyser. I find them to be such powerful tools to have on top of PHP, it wouldn't cross my mind doing without them nowadays.

[BenMorel]

I think I'm not ready to accept null values everywhere, @gnutix.

I must admit I tend to forget that not everybody use and rely on a static analyser. I find them to be such powerful tools to have on top of PHP, it wouldn't cross my mind doing without them nowadays.

No doubt about this, everybody should use one. But not everybody does, plus the @return ($a is null ? ($b is null ? null : Duration) : Duration) is a bit hard to read for a human to understand what the method really does at first glance.

---

Or at least a toString method without the magic method's __.

I may revisit my opinion about this one. I recently had to use ?->__toString(), and:

• I agree that it looks ugly, and in general magic methods are not really meant to be used explicitly
• I noticed that PhpStorm does not suggest magic methods when you type ->

@tigitz Do you have an opinion on this?

Basically we would end up with something like:

public function toString(): string
{
    // ...
}

public function jsonSerialize(): string
{
    return $this->toString();
}

public function __toString(): string
{
    return $this->toString();
}

That's quite a lot of redundancy, but each one does have its use cases.

[gnutix]

I think I'm not ready to accept null values everywhere

The number one places that would benefit from this are the static constructor methods (especially the parse methods). But I get your point, and if you don't feel like it, that's fine. I can always composer patch it up for our needs.

About the 3 toString methods, I'd still rather have an iso method over toString, because toString is very "technical" (cast the object into a string), while iso is more "business-related": we want an ISO-8601 representation of the object.

[tigitz]

@BenMorel To me it's fine to have all of them, including the suggested toISOString(). We also use a lot of __toString() in our codebase and it itches.

The added maintenance complexity is far outweighed by the additional expressiveness in the api IMO, allowing users to properly define what they really want from the Duration object.

Popular Days.js library also has those methods, sometimes returning the same output:

• https://day.js.org/docs/en/display/as-iso-string
• https://day.js.org/docs/en/display/as-json
• https://day.js.org/docs/en/display/as-string

[BenMorel]

@tigitz Thank you for the pointer. I'm fine with toISOString (or toIsoString, it's always hard to decide on acronym casing!)

@gnutix Do you want to make a PR?

[gnutix]

@BenMorel I am working on it, but I really don't see the value of adding *String() in the method name. The ISO representation cannot be anything else than a string. And the type hint already says what type it will be. Could we simply name it toISO() ? (which solves the casing matter too, and makes it simpler/shorter)

[gnutix]

I pushed 2 commits on #85 (as I already fixed a bug in YearMonth::__toString() there) adding toISO() methods.

• I've skipped TimeZoneRegion and TimeZoneOffset, as they already have a getId(): string method.
• For Month, I named the method getName(), as toISO() felt wrong.

Please move the conversation there. 🙏

[gnutix]

Closing as we won't do anything about accepting null arguments in parse methods and such.