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

Using Doctrine LifecycleCallbacks #1439

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Using Doctrine LifecycleCallbacks #1439

wants to merge 7 commits into from

Conversation

Mecanik
Copy link

@Mecanik Mecanik commented Oct 20, 2023

This is the modern solution to automatically update timestamps in Doctrine. I have added createdAt and updatedAt to all entities using LifecycleCallbacks.

This is the modern solution to automatically update timestamps in Doctrine. I have added createdAt and updatedAt to all entities using LifecycleCallbacks.
Comment on lines 104 to 106
if ($this->getUpdatedAt() == null) {
$this->setUpdatedAt(new \DateTimeImmutable('now'));
}
Copy link
Member

Choose a reason for hiding this comment

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

The null check does not make sense here. With this logic, we will basically set the updatedAt once when creating the record and never update it.

Copy link
Author

Choose a reason for hiding this comment

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

True, sorry as mentioned I copied from an older project that needed only one time to update.

Comment on lines 107 to 109
if ($this->getCreatedAt() == null) {
$this->setCreatedAt(new \DateTimeImmutable('now'));
}
Copy link
Member

Choose a reason for hiding this comment

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

Why does a PreUpdate handler deal with the createdAt property? That property should by definition be set once and never be updated.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I just copied from an older project. The point was to demonstrate how it can be used and should be used in an official demo. I will update shortly.

{
$this->updatedAt = $updatedAt;

return $this;
Copy link
Member

Choose a reason for hiding this comment

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

there is no reason to make those setters fluent. Other setters in the entity are not.

Copy link
Member

Choose a reason for hiding this comment

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

More over, there is not reason to add theses public method

composer.json Outdated
@@ -50,6 +50,7 @@
"require-dev": {
"dama/doctrine-test-bundle": "^7.0",
"doctrine/doctrine-fixtures-bundle": "^3.4",
"friendsofphp/php-cs-fixer": "^3.35",
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated change, please revert.

@@ -51,6 +56,12 @@ class Comment
#[ORM\JoinColumn(nullable: false)]
private ?User $author = null;

#[ORM\Column(type: 'datetime_immutable')]
private $createdAt;
Copy link
Member

Choose a reason for hiding this comment

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

Our linters rightfully complain about missing property types. Please add them.

@@ -51,6 +56,12 @@ class Comment
#[ORM\JoinColumn(nullable: false)]
private ?User $author = null;

#[ORM\Column(type: Types::DATETIMETZ_IMMUTABLE)]
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that we want to use the timezone-aware datetime field here.

@94noni
Copy link
Contributor

94noni commented Oct 23, 2023

hello @Mecanik , I like the PR and I also use doctrine logic, but I fail/dont get what this PR adds as value on the side of this "symfony" demo?
perhaps just add some "url doc" on top of the entities and point to doctrine may be enough ?

@Mecanik
Copy link
Author

Mecanik commented Oct 24, 2023

hello @Mecanik , I like the PR and I also use doctrine logic, but I fail/dont get what this PR adds as value on the side of this "symfony" demo? perhaps just add some "url doc" on top of the entities and point to doctrine may be enough ?

Thanks. My idea is to guide users to use not just Symfony "the right way" but it's core components like Doctrine. Realistically, nobody will read the doc unless they have to. I have seen thousands of projects developed the "same way" without any improvements or using modern functionality. I would like the demo to slowly become as modern as possible, using latest features, so when a newbie starts coding he follows the right path.

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.

5 participants