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.

Sign up for GitHub

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

Jump to bottom

Allow DateTimeImmutable be part of composite key #10830

Open
wants to merge 11 commits into
base: 2.15.x
Choose a base branch
from
Open

Allow DateTimeImmutable be part of composite key #10830

wants to merge 11 commits into from

Conversation

Greg0
Copy link
Contributor

@Greg0 Greg0 commented Jul 10, 2023 •
edited
Loading

I've added implementation for the attached previously use-case.
The whole description can be found in the linked issue #10831

@Greg0 Greg0 mentioned this pull request Jul 10, 2023
Open
@Greg0 Greg0 changed the title Add use case of datetime as composite key part Add use case of datetime immutable as composite key part Jul 17, 2023
@Greg0 Greg0 marked this pull request as ready for review July 17, 2023 07:03
@Greg0 Greg0 changed the title Add use case of datetime immutable as composite key part Allow DateTimeImmutable be part of composite key Jul 17, 2023
@greg0ire
Copy link
Member

There are some github action checks that should be run again to handle changes.

What do you mean?

@greg0ire
Copy link
Member

You may disregard the documentation build, it's not stable yet.

@Greg0
Copy link
Contributor Author

Greg0 commented Jul 30, 2023

When can I expect a review of PR?

greg0ire
greg0ire reviewed Jul 30, 2023
View reviewed changes
Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

Codecov reported 2 blocks of code as uncovered, did you notice that?

docs/en/tutorials/composite-primary-keys.rst Show resolved Hide resolved
tests/Doctrine/Tests/ORM/Utility/IdentifierFlattenerDateTimeImmutableIdTest.php Outdated
{
$article = new Article('Some title');
$article->changeTitle('New title');
sleep(1);
Copy link
Member

Choose a reason for hiding this comment

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

Please find a way to avoid this. Maybe define a class that extends DateTimeImmutable and makes sure to add 1 second when the constructor is called, I don't know.

BTW, doesn't this point at a weakness of having time part of the composite key? What happens in high throughput scenarios? People might end up with entities with the same ID, might they not? I'm starting to have some doubts about this…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I consider such validation as a part of entity design. Ofc I can add that validation in tested entities to get rid of that sleep function.

It is not the responsibility of Doctrine to think for programmers on the consistency of their entities.

Copy link
Member

Choose a reason for hiding this comment

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

It's not that I want to tell people what's wrong or right, but I would like to avoid people opening bug reports, have us help them debug only to find they are hitting this limitation. That's why I'm weary of code that works only if people know what they are doing.

Copy link
Contributor Author

@Greg0 Greg0 Aug 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Agree but requirement of having unique entity identifier is not magic knowledge when you are dealing with ORM and even database I guess. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

It sounds sensible. Let's keep that thread open, and see what other maintainers think.

@Greg0
Copy link
Contributor Author

Greg0 commented Jul 31, 2023

Codecov reported 2 blocks of code as uncovered, did you notice that?

I've only noticed issues with documentation checks
obraz

@greg0ire
Copy link
Member

I've only noticed issues with documentation checks

It appears in the form of comments on the files tab, when you have a successful build (which explains why it isn't available right now).

@Greg0
Copy link
Contributor Author

Greg0 commented Jul 31, 2023

I've only noticed issues with documentation checks

It appears in the form of comments on the files tab, when you have a successful build (which explains why it isn't available right now).

Everything passed now

@greg0ire
Copy link
Member

This is what I'm talking about:

2023-07-31_17-02

@Greg0
Copy link
Contributor Author

Greg0 commented Aug 1, 2023

@greg0ire Can you point me to a place where tests that cover UnitOfWork::createEntity are located? Or where should I place it? I looked for them and didn't find anything promising.

@greg0ire
Copy link
Member

greg0ire commented Aug 1, 2023

I'm on my phone so no. Have you tried throwing an exception in nearby code and running the test suite.

@Greg0
Copy link
Contributor Author

Greg0 commented Aug 1, 2023

I'm on my phone so no. Have you tried throwing an exception in nearby code and running the test suite.

I figured out how to check that part of the code. I need more time for that and I'll let you know when it will be finished

@arkhamvm
Copy link

Any news? Faced same problem with DateTime in 3.x branch.

@dpi dpi mentioned this pull request Sep 14, 2024
Open
@github-actions GitHub Actions
Copy link
Contributor

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

@github-actions github-actions bot added the Stale label Oct 22, 2024
@arkhamvm
Copy link

Any news? Faced same problem with DateTime in 3.x branch.

Still actual problem.

@Greg0
Copy link
Contributor Author

Greg0 commented Oct 22, 2024 •
edited
Loading

I'm faced too many issues in entity management when working on covering code with tests so I gave up. Maybe someone else based on my changes will have better idea how to solve it proper.

AFAIR main issue was: to treat datetime field as part of key you must cast it to string, but you can't cast it to string because it has to be object field in other processes like persistence. With enum type it was easier because enum in fact is regular field with type=string which is only marked in mapping as enum driven https://www.doctrine-project.org/projects/doctrine-laminas-hydrator/en/3.4/enum-strategy.html#the-enum-and-entity

@github-actions github-actions bot removed the Stale label Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greg0ire greg0ire greg0ire left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Greg0 @greg0ire @arkhamvm