Skip to content

Update Subscribers with #[AsDoctrineListener] #742

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Update Subscribers with #[AsDoctrineListener] #742

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ee0170e
WIP
DennisdeBest Oct 20, 2023
2b26634
Update Subscribers, Update Rector
DennisdeBest Oct 20, 2023
64c49b5
Set to minimum PHP 8.1 Add Symfony 7 support
DennisdeBest Feb 25, 2024
cd825fa
Set to Symfony 6.1 minimum
DennisdeBest Jun 8, 2024
ec2ccd5
Update DoctrineBehaviors for Symfony 7 and Doctrine ORM 3 compatibility
DennisdeBest Dec 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/code_checks.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,10 @@ jobs:
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v2
- uses: shivammathur/setup-php@v1
- uses: actions/checkout@v4
- uses: shivammathur/setup-php@v2
with:
php-version: 8.0
php-version: 8.1
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be reverted as Symfony 6.0 still supports PHP 8.0.
If you want to add a test on PHP 8.1 you should add it instead of just change it to it IMHO.

Copy link

Choose a reason for hiding this comment

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

symfony 6.0 is "Unmaintained" https://symfony.com/releases/6.0
symfony 6.4 is maintained ATM and: "Requires: PHP 8.1.0 or higher"

extensions: pdo_sqlite, pdo_mysql, pdo_pgsql
coverage: none

Expand Down
6 changes: 3 additions & 3 deletions .github/workflows/linter.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,11 @@ jobs:
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v2
- uses: actions/checkout@v4

- uses: shivammathur/setup-php@v1
- uses: shivammathur/setup-php@v2
with:
php-version: 8.0
php-version: 8.1
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be reverted as Symfony 6.0 still supports PHP 8.0.
If you want to add a test on PHP 8.1 you should add it instead of just change it to it IMHO.

extensions: pdo_sqlite, pdo_mysql, pdo_pgsql
coverage: none

Expand Down
8 changes: 4 additions & 4 deletions .github/workflows/tests.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,16 @@ jobs:
tests:
strategy:
matrix:
php:
- 8.0
operating-system: [ ubuntu-latest ]
php-versions: [ '8.1', '8.3' ]
Copy link
Contributor

Choose a reason for hiding this comment

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

We should still test php 8.0 as well for the same reasons.


name: Test ${{ matrix.php }}
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v2
- uses: actions/checkout@v4

- uses: shivammathur/setup-php@v1
- uses: shivammathur/setup-php@v2
with:
php-version: ${{ matrix.php }}
extensions: pdo_sqlite, pdo_mysql, pdo_pgsql
Expand Down
18 changes: 9 additions & 9 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,18 @@
{ "name": "Knplabs", "homepage": "http://knplabs.com" }
],
"require": {
"php": ">=8.0",
"php": ">=8.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be reverted as Symfony 6 still supports PHP 8.0 unless there is a reason for it.

"doctrine/common": "^3.3",
"doctrine/persistence": "^2.5|^3.0",
"doctrine/dbal": "^3.3",
"doctrine/orm": "^2.12",
"doctrine/doctrine-bundle": "^2.6",
"symfony/cache": "^5.4|^6.0",
"symfony/dependency-injection": "^5.4|^6.0",
"symfony/http-kernel": "^5.4|^6.0",
"symfony/security-core": "^5.4|^6.0",
"symfony/framework-bundle": "^5.4|^6.0",
"symfony/string": "^5.4|^6.0",
"doctrine/doctrine-bundle": "^2.7.2",
"symfony/cache": "^6.0|^7.0",
"symfony/dependency-injection": "^6.0|^7.0",
"symfony/http-kernel": "^6.0|^7.0",
"symfony/security-bundle": "^6.0|^7.0",
"symfony/framework-bundle": "^6.0|^7.0",
"symfony/string": "^6.0|^7.0",
Copy link

Choose a reason for hiding this comment

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

Why are versions 6.0-6.3, which are EOL, supported, while LTS version 5.4, which has support until November 2024 and security support until November 2025, is not?

Copy link
Contributor

Choose a reason for hiding this comment

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

It can be decided to introduce a breaking change in the library (like this one) and create a new major version while still supporting 5.4 (patches only) in the previous major version.
The fact that we bump the requirements here does not mean that the library stops supporting Symfony 5.4.

Supporting 6.0-6.3 makes sense as well unless there are some constraint that are creating us issues IMHO.
When writing libraries it's better to give a wider support of versions if there are no real reasons not to do it IMHO, not everyone might have upgraded yet to the latest 6.x version.

"symfony/translation-contracts": "^2.4|^3.0",
"nette/utils": "^3.2",
"ramsey/uuid": "^4.2"
Expand All @@ -36,7 +36,7 @@
"php-parallel-lint/php-parallel-lint": "^1.3",
"phpstan/phpstan": "^1.7.10",
"phpunit/phpunit": "^9.5",
"rector/rector": "^0.13.4",
"rector/rector": "^1.0.0",
"symplify/easy-coding-standard": "^10.2.9",
"symplify/phpstan-extensions": "^10.2.9",
"phpstan/phpstan-doctrine": "^1.3",
Expand Down
11 changes: 2 additions & 9 deletions rector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,9 @@
use Rector\Config\RectorConfig;
use Rector\Doctrine\Set\DoctrineSetList;
use Rector\Naming\Rector\Class_\RenamePropertyToMatchTypeRector;
use Rector\Nette\Set\NetteSetList;
use Rector\Php80\Rector\FunctionLike\UnionTypesRector;
use Rector\Set\ValueObject\LevelSetList;
use Rector\Set\ValueObject\SetList;
use Rector\TypeDeclaration\Rector\Property\TypedPropertyFromAssignsRector;

return static function (RectorConfig $rectorConfig): void {
$rectorConfig->paths([__DIR__ . '/src', __DIR__ . '/tests', __DIR__ . '/utils']);
Expand All @@ -17,12 +16,7 @@

$rectorConfig->skip([
RenamePropertyToMatchTypeRector::class => [__DIR__ . '/tests/ORM/'],

UnionTypesRector::class => [
// to keep BC return types
__DIR__ . '/src/Contract/Entity',
'src/Model/*/*Trait.php',
],
TypedPropertyFromAssignsRector::class => [__DIR__ . '/tests/Repository/'],
]);

// doctrine annotations to attributes
Expand All @@ -31,7 +25,6 @@
SetList::DEAD_CODE,
SetList::CODE_QUALITY,
SetList::CODING_STYLE,
NetteSetList::NETTE_CODE_QUALITY,
SetList::NAMING,
LevelSetList::UP_TO_PHP_80,
]);
Expand Down
2 changes: 1 addition & 1 deletion src/Bundle/DependencyInjection/DoctrineBehaviorsExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
final class DoctrineBehaviorsExtension extends Extension
{
/**
* @param string[] $configs
* @param array<array<mixed>> $configs
*/
public function load(array $configs, ContainerBuilder $containerBuilder): void
{
Expand Down
62 changes: 30 additions & 32 deletions src/EventSubscriber/BlameableEventSubscriber.php
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep $entity instead of changing it to $object as it's more specific to this case.

Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,23 @@

namespace Knp\DoctrineBehaviors\EventSubscriber;

use Doctrine\Bundle\DoctrineBundle\EventSubscriber\EventSubscriberInterface;
use Doctrine\Bundle\DoctrineBundle\Attribute\AsDoctrineListener;
use Doctrine\ORM\EntityManagerInterface;
use Doctrine\ORM\Event\LifecycleEventArgs;
use Doctrine\ORM\Event\LoadClassMetadataEventArgs;
use Doctrine\ORM\Event\PrePersistEventArgs;
use Doctrine\ORM\Event\PreRemoveEventArgs;
use Doctrine\ORM\Event\PreUpdateEventArgs;
use Doctrine\ORM\Events;
use Doctrine\ORM\Mapping\ClassMetadataInfo;
use Doctrine\ORM\UnitOfWork;
use Knp\DoctrineBehaviors\Contract\Entity\BlameableInterface;
use Knp\DoctrineBehaviors\Contract\Provider\UserProviderInterface;

final class BlameableEventSubscriber implements EventSubscriberInterface
#[AsDoctrineListener(event: Events::loadClassMetadata)]
#[AsDoctrineListener(event: Events::prePersist)]
#[AsDoctrineListener(event: Events::preUpdate)]
#[AsDoctrineListener(event: Events::preRemove)]
final class BlameableEventSubscriber
{
/**
* @var string
Expand Down Expand Up @@ -59,10 +65,10 @@ public function loadClassMetadata(LoadClassMetadataEventArgs $loadClassMetadataE
/**
* Stores the current user into createdBy and updatedBy properties
*/
public function prePersist(LifecycleEventArgs $lifecycleEventArgs): void
public function prePersist(PrePersistEventArgs $prePersistEventArgs): void
{
$entity = $lifecycleEventArgs->getEntity();
if (! $entity instanceof BlameableInterface) {
$object = $prePersistEventArgs->getObject();
if (! $object instanceof BlameableInterface) {
return;
}

Expand All @@ -72,28 +78,28 @@ public function prePersist(LifecycleEventArgs $lifecycleEventArgs): void
return;
}

if (! $entity->getCreatedBy()) {
$entity->setCreatedBy($user);
if (! $object->getCreatedBy()) {
$object->setCreatedBy($user);

$this->getUnitOfWork()
->propertyChanged($entity, self::CREATED_BY, null, $user);
->propertyChanged($object, self::CREATED_BY, null, $user);
}

if (! $entity->getUpdatedBy()) {
$entity->setUpdatedBy($user);
if (! $object->getUpdatedBy()) {
$object->setUpdatedBy($user);

$this->getUnitOfWork()
->propertyChanged($entity, self::UPDATED_BY, null, $user);
->propertyChanged($object, self::UPDATED_BY, null, $user);
}
}

/**
* Stores the current user into updatedBy property
*/
public function preUpdate(LifecycleEventArgs $lifecycleEventArgs): void
public function preUpdate(PreUpdateEventArgs $preUpdateEventArgs): void
{
$entity = $lifecycleEventArgs->getEntity();
if (! $entity instanceof BlameableInterface) {
$object = $preUpdateEventArgs->getObject();
if (! $object instanceof BlameableInterface) {
return;
}

Expand All @@ -102,20 +108,20 @@ public function preUpdate(LifecycleEventArgs $lifecycleEventArgs): void
return;
}

$oldValue = $entity->getUpdatedBy();
$entity->setUpdatedBy($user);
$updatedBy = $object->getUpdatedBy();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$updatedBy = $object->getUpdatedBy();
$oldUpdatedBy = $object->getUpdatedBy();

$object->setUpdatedBy($user);

$this->getUnitOfWork()
->propertyChanged($entity, self::UPDATED_BY, $oldValue, $user);
->propertyChanged($object, self::UPDATED_BY, $updatedBy, $user);
}

/**
* Stores the current user into deletedBy property
*/
public function preRemove(LifecycleEventArgs $lifecycleEventArgs): void
public function preRemove(PreRemoveEventArgs $preRemoveEventArgs): void
{
$entity = $lifecycleEventArgs->getEntity();
if (! $entity instanceof BlameableInterface) {
$object = $preRemoveEventArgs->getObject();
if (! $object instanceof BlameableInterface) {
return;
}

Expand All @@ -124,19 +130,11 @@ public function preRemove(LifecycleEventArgs $lifecycleEventArgs): void
return;
}

$oldDeletedBy = $entity->getDeletedBy();
$entity->setDeletedBy($user);
$oldDeletedBy = $object->getDeletedBy();
$object->setDeletedBy($user);

$this->getUnitOfWork()
->propertyChanged($entity, self::DELETED_BY, $oldDeletedBy, $user);
}

/**
* @return string[]
*/
public function getSubscribedEvents(): array
{
return [Events::prePersist, Events::preUpdate, Events::preRemove, Events::loadClassMetadata];
->propertyChanged($object, self::DELETED_BY, $oldDeletedBy, $user);
}

private function mapEntity(ClassMetadataInfo $classMetadataInfo): void
Expand Down
61 changes: 29 additions & 32 deletions src/EventSubscriber/LoggableEventSubscriber.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,77 +4,74 @@

namespace Knp\DoctrineBehaviors\EventSubscriber;

use Doctrine\Bundle\DoctrineBundle\EventSubscriber\EventSubscriberInterface;
use Doctrine\ORM\Event\LifecycleEventArgs;
use Doctrine\Bundle\DoctrineBundle\Attribute\AsDoctrineListener;
use Doctrine\ORM\Event\PostPersistEventArgs;
use Doctrine\ORM\Event\PostUpdateEventArgs;
use Doctrine\ORM\Event\PreRemoveEventArgs;
use Doctrine\ORM\Events;
use Knp\DoctrineBehaviors\Contract\Entity\LoggableInterface;
use Psr\Log\LoggerInterface;
use Psr\Log\LogLevel;

final class LoggableEventSubscriber implements EventSubscriberInterface
#[AsDoctrineListener(event: Events::postPersist)]
#[AsDoctrineListener(event: Events::postUpdate)]
#[AsDoctrineListener(event: Events::preRemove)]
final class LoggableEventSubscriber
{
public function __construct(
private LoggerInterface $logger
) {
}

public function postPersist(LifecycleEventArgs $lifecycleEventArgs): void
public function postPersist(PostPersistEventArgs $postPersistEventArgs): void
{
$entity = $lifecycleEventArgs->getEntity();
if (! $entity instanceof LoggableInterface) {
$object = $postPersistEventArgs->getObject();
if (! $object instanceof LoggableInterface) {
return;
}

$createLogMessage = $entity->getCreateLogMessage();
$createLogMessage = $object->getCreateLogMessage();
$this->logger->log(LogLevel::INFO, $createLogMessage);

$this->logChangeSet($lifecycleEventArgs);
$this->logChangeSet($postPersistEventArgs);
}

public function postUpdate(LifecycleEventArgs $lifecycleEventArgs): void
public function postUpdate(PostUpdateEventArgs $postUpdateEventArgs): void
{
$entity = $lifecycleEventArgs->getEntity();
if (! $entity instanceof LoggableInterface) {
$object = $postUpdateEventArgs->getObject();
if (! $object instanceof LoggableInterface) {
return;
}

$this->logChangeSet($lifecycleEventArgs);
$this->logChangeSet($postUpdateEventArgs);
}

public function preRemove(LifecycleEventArgs $lifecycleEventArgs): void
public function preRemove(PreRemoveEventArgs $preRemoveEventArgs): void
{
$entity = $lifecycleEventArgs->getEntity();
$object = $preRemoveEventArgs->getObject();

if ($entity instanceof LoggableInterface) {
$this->logger->log(LogLevel::INFO, $entity->getRemoveLogMessage());
if ($object instanceof LoggableInterface) {
$this->logger->log(LogLevel::INFO, $object->getRemoveLogMessage());
}
}

/**
* @return string[]
*/
public function getSubscribedEvents(): array
{
return [Events::postPersist, Events::postUpdate, Events::preRemove];
}

/**
* Logs entity changeset
*/
private function logChangeSet(LifecycleEventArgs $lifecycleEventArgs): void
private function logChangeSet(PostPersistEventArgs|PostUpdateEventArgs $lifecycleEventArgs): void
{
$entityManager = $lifecycleEventArgs->getEntityManager();
$entityManager = $lifecycleEventArgs->getObjectManager();
$unitOfWork = $entityManager->getUnitOfWork();
$entity = $lifecycleEventArgs->getEntity();
$object = $lifecycleEventArgs->getObject();

$entityClass = $entity::class;
$entityClass = $object::class;
$classMetadata = $entityManager->getClassMetadata($entityClass);

/** @var LoggableInterface $entity */
$unitOfWork->computeChangeSet($classMetadata, $entity);
$changeSet = $unitOfWork->getEntityChangeSet($entity);
/** @var LoggableInterface $object */
$unitOfWork->computeChangeSet($classMetadata, $object);
$changeSet = $unitOfWork->getEntityChangeSet($object);

$message = $entity->getUpdateLogMessage($changeSet);
$message = $object->getUpdateLogMessage($changeSet);

if ($message === '') {
return;
Expand Down
Loading