Compare time value objects by value, never by instance. - #20
Merged
Conversation
Instant and LocalDate declared the value-object contract while holding a DateTimeImmutable, and structural equality compares a property that is not itself a value object with ===, which on an object is identity. Two instances of the same moment were never equal, their hash codes never matched, and neither did any value object that carried one as a property. Duration had the same defect through Internal\Seconds, and Timezone did not declare the contract at all. Overriding equals would not have been enough: ValueObjectEquality walks the properties by reflection and never calls equals, so a wrapper would keep comparing the inner DateTimeImmutable by identity. The state is what had to change, so Instant and LocalDate now hold their canonical text, and IsoMoment and IsoDate own the conversion in Internal, where the style rule allows the helpers. Fixing LocalDate surfaced a second defect. Its parser filled the time of day from the clock of the moment it ran, so two instances of the same date built a second apart reported isAfter and isBefore as true at the same time. Holding the canonical date removes the wall clock along with the identity comparison. Both createFromFormat calls are gone with it, which removed one PHPStan suppression instead of adding another. Two notes for consumers. Timezone gains equals and hashCode, which is new public surface. toDateTimeImmutable now returns a new object on every call rather than the stored one, so identity comparisons of its return value change; value comparisons and every other behaviour are unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Instant and LocalDate declared the value-object contract while holding a DateTimeImmutable, and structural equality compares a property that is not itself a value object with ===, which on an object is identity. Two instances of the same moment were never equal, their hash codes never matched, and neither did any value object that carried one as a property. Duration had the same defect through Internal\Seconds, and Timezone did not declare the contract at all.
Overriding equals would not have been enough: ValueObjectEquality walks the properties by reflection and never calls equals, so a wrapper would keep comparing the inner DateTimeImmutable by identity. The state is what had to change, so Instant and LocalDate now hold their canonical text, and IsoMoment and IsoDate own the conversion in Internal, where the style rule allows the helpers.
Fixing LocalDate surfaced a second defect. Its parser filled the time of day from the clock of the moment it ran, so two instances of the same date built a second apart reported isAfter and isBefore as true at the same time. Holding the canonical date removes the wall clock along with the identity comparison.
Both createFromFormat calls are gone with it, which removed one PHPStan suppression instead of adding another.
Two notes for consumers. Timezone gains equals and hashCode, which is new public surface. toDateTimeImmutable now returns a new object on every call rather than the stored one, so identity comparisons of its return value change; value comparisons and every other behaviour are unchanged.
Summary
What this pull request does.
Related issue
Closes #...
Checklist
make reviewpasses.make testspasses.