Skip to content

Using Doctrine LifecycleCallbacks - #1439

Open
Mecanik wants to merge 7 commits into
symfony:mainfrom
Mecanik:main
Open

Using Doctrine LifecycleCallbacks#1439
Mecanik wants to merge 7 commits into
symfony:mainfrom
Mecanik:main

Conversation

@Mecanik

@Mecanik Mecanik commented Oct 20, 2023

Copy link
Copy Markdown

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 thread src/Entity/Tag.php Outdated
Comment on lines +104 to +106
if ($this->getUpdatedAt() == null) {
$this->setUpdatedAt(new \DateTimeImmutable('now'));
}

Copy link
Copy Markdown
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
Copy Markdown
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 thread src/Entity/Tag.php Outdated
Comment on lines +107 to +109
if ($this->getCreatedAt() == null) {
$this->setCreatedAt(new \DateTimeImmutable('now'));
}

Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread src/Entity/Comment.php
{
$this->updatedAt = $updatedAt;

return $this;

Copy link
Copy Markdown
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
Copy Markdown
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

Comment thread composer.json Outdated
"require-dev": {
"dama/doctrine-test-bundle": "^7.0",
"doctrine/doctrine-fixtures-bundle": "^3.4",
"friendsofphp/php-cs-fixer": "^3.35",

Copy link
Copy Markdown
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.

Comment thread src/Entity/Comment.php
private ?User $author = null;

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

Copy link
Copy Markdown
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.

Comment thread src/Entity/Comment.php Outdated
#[ORM\JoinColumn(nullable: false)]
private ?User $author = null;

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

Copy link
Copy Markdown
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

94noni commented Oct 23, 2023

Copy link
Copy Markdown
Contributor

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

Mecanik commented Oct 24, 2023

Copy link
Copy Markdown
Author

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