brick / date-time

Date and time library for PHP
MIT License
337 stars 30 forks source link

Set ID of TimeZone offset on construct #43

Closed Robertbaelde closed 2 years ago

Robertbaelde commented 2 years ago

In order to compare TimeZone Offset reliably, the id must be set on construction. Otherwise, when objects including timezones are compared, a test would fail because in some cases the id has been set. And in other cases, the id hasn't been set.

Previously this test would fail:

public function testIdIsSetOnInstantiation(): void
    {
        $timezone = TimeZoneOffset::utc();
        $parsedTimezone = TimeZoneOffset::parse((string) $timezone);

        $this->assertEquals($timezone, $parsedTimezone);
    }
BenMorel commented 2 years ago

Hi @Robertbaelde, this is an optimization as the ID does not always need to be computed.

My stance of this is that you should never compare objects by value, as you're making yourself implicitly dependent on class internals, that are not exposed by the public API. This is something IMHO that PHP should forbid, or at least make harder to do than ==.

You should instead use the API that the library provides, i.e. TimeZone::isEqualTo().

But keep an eye on #55, it's not bulletproof yet 😕