Skip to content

[Uid] Make AbstractUid implement Ds\Hashable if available #57313

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

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

jahudka
Copy link
Contributor

@jahudka jahudka commented Jun 4, 2024

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? no
License MIT

This PR makes UUIDs usable with the PHP DS extension's Map and Set classes. It's a new feature and shouldn't cause any BC breaks.

The Map and Set classes in the DS extension have extended support for objects implementing the Ds\Hashable interface (as keys in Map and values in Set). By default, values are made unique by testing for strict equality, which will consider two Uuid instances to be distinct even though they represent the same UUID; with this PR merged, Uuid instances would behave as expected with these classes.

The current implementation of AbstractUid already includes the equals() method defined in the interface; the PR adds the missing hash() method and a shim for the interface in case the extension isn't loaded. I've also added a test for the new hash() method, checking both the behaviour of the method itself and the correctness of that behaviour when used with the Ds\Set class (that part of the test only runs if the ds extension is loaded).

I've read through the contributing docs, but it's still my first time contributing to Symfony, I'm sure I've made mistakes - please don't be too harsh on me! 🙏 ❤️

@carsonbot

This comment was marked as resolved.

@jahudka jahudka force-pushed the feat-uid-ds-hashable branch 4 times, most recently from b417d14 to 055e71a Compare June 4, 2024 14:42
@OskarStark OskarStark changed the title [Uid] Make AbstractUid implement Ds\Hashable if available [Uid] Make AbstractUid implement Ds\Hashable if available Jun 5, 2024
@jahudka jahudka force-pushed the feat-uid-ds-hashable branch from 055e71a to 878da02 Compare June 5, 2024 08:14
@symfony symfony deleted a comment from carsonbot Jun 5, 2024
@jahudka jahudka force-pushed the feat-uid-ds-hashable branch from 878da02 to 9b9dbb9 Compare June 5, 2024 08:46
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we use psalm as a tool that could give useful hints, but that could also generate false-positives 🤷

@jahudka jahudka force-pushed the feat-uid-ds-hashable branch from 9b9dbb9 to 1a0d9b0 Compare June 5, 2024 09:41
@fabpot
Copy link
Member

fabpot commented Jun 7, 2024

Thank you @jahudka.

@fabpot fabpot merged commit 63ef48f into symfony:7.2 Jun 7, 2024
8 of 10 checks passed
@fabpot fabpot mentioned this pull request Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants