Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set ID of TimeZone offset on construct #43

Conversation

Robertbaelde
Copy link

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);
    }

In order to compare TimeZone Offset reliably, the id must be set on construction.
@BenMorel
Copy link
Member

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 😕

@BenMorel BenMorel closed this Jul 11, 2022
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.

2 participants