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

Blameable and Frankenphp creates a new entity #2843

Open
tuxes3 opened this issue Jul 17, 2024 · 2 comments
Open

Blameable and Frankenphp creates a new entity #2843

tuxes3 opened this issue Jul 17, 2024 · 2 comments

Comments

@tuxes3
Copy link

tuxes3 commented Jul 17, 2024

Environment

Package

show

$ composer show --latest gedmo/doctrine-extensions
name     : gedmo/doctrine-extensions
descrip. : Doctrine behavioral extensions
keywords : Blameable, behaviors, doctrine, extensions, gedmo, loggable, nestedset, odm, orm, sluggable, sortable, timestampable, translatable, tree, uploadable
versions : * v3.15.0
released : 2024-02-12, 5 months ago
latest   : v3.16.1 released 2024-06-25, 3 weeks ago
type     : library
license  : MIT License (MIT) (OSI approved) https://spdx.org/licenses/MIT.html#licenseText
homepage : http://gediminasm.org/
source   : [git] https://github.com/doctrine-extensions/DoctrineExtensions.git 2a89103f4984d8970f3855284c8c04e6e6a63c0f
dist     : [zip] https://api.github.com/repos/doctrine-extensions/DoctrineExtensions/zipball/2a89103f4984d8970f3855284c8c04e6e6a63c0f 2a89103f4984d8970f3855284c8c04e6e6a63c0f
path     : /var/www/vendor/gedmo/doctrine-extensions
names    : gedmo/doctrine-extensions

support
email : [email protected]
issues : https://github.com/doctrine-extensions/DoctrineExtensions/issues
source : https://github.com/doctrine-extensions/DoctrineExtensions/tree/v3.15.0
wiki : https://github.com/Atlantic18/DoctrineExtensions/tree/main/doc

autoload
psr-4
Gedmo\ => src/

requires
behat/transliterator ^1.2
doctrine/collections ^1.2 || ^2.0
doctrine/common ^2.13 || ^3.0
doctrine/deprecations ^1.0
doctrine/event-manager ^1.2 || ^2.0
doctrine/persistence ^2.2 || ^3.0
php ^7.4 || ^8.0
psr/cache ^1 || ^2 || ^3
psr/clock ^1
symfony/cache ^5.4 || ^6.0 || ^7.0

requires (dev)
doctrine/annotations ^1.13 || ^2.0
doctrine/cache ^1.11 || ^2.0
doctrine/dbal ^3.2
doctrine/doctrine-bundle ^2.3
doctrine/mongodb-odm ^2.3
doctrine/orm ^2.14.0
friendsofphp/php-cs-fixer ^3.14.0
nesbot/carbon ^2.71 || ^3.0
phpstan/phpstan ^1.10.2
phpstan/phpstan-doctrine ^1.0
phpstan/phpstan-phpunit ^1.0
phpunit/phpunit ^9.6
rector/rector ^0.19
symfony/console ^5.4 || ^6.0 || ^7.0
symfony/phpunit-bridge ^6.0 || ^7.0
symfony/yaml ^5.4 || ^6.0 || ^7.0

suggests
doctrine/mongodb-odm to use the extensions with the MongoDB ODM
doctrine/orm to use the extensions with the ORM

conflicts
doctrine/annotations <1.13 || >=3.0
doctrine/dbal <3.2 || >=4.0
doctrine/mongodb-odm <2.3 || >=3.0
doctrine/orm <2.14.0 || 2.16.0 || 2.16.1 || >=3.0

Doctrine packages

show

$ composer show --latest 'doctrine/*'
Direct dependencies required in composer.json:
doctrine/annotations                2.0.1  2.0.1  Docblock Annotations Parser
doctrine/dbal                       3.8.4  4.0.4  Powerful PHP database abstraction layer (DBAL) with many features for database schema introspection and management.
doctrine/doctrine-bundle            2.12.0 2.12.0 Symfony DoctrineBundle
doctrine/doctrine-fixtures-bundle   3.6.1  3.6.1  Symfony DoctrineFixturesBundle
doctrine/doctrine-migrations-bundle 3.3.1  3.3.1  Symfony DoctrineMigrationsBundle
doctrine/migrations                 3.7.4  3.8.0  PHP Doctrine Migrations project offer additional functionality on top of the database abstraction layer (DBAL) for versioning your database schema and easily de...
doctrine/orm                        2.19.5 3.2.1  Object-Relational-Mapper for PHP
doctrine/persistence                3.3.2  3.3.3  The Doctrine Persistence project is a set of shared interfaces and functionality that the different Doctrine object mappers share.

Transitive dependencies not required in composer.json:
doctrine/cache                      2.2.0  2.2.0  PHP Doctrine Cache library is a popular cache implementation that supports many different drivers such as redis, memcache, apc, mongodb and others.
doctrine/collections                2.2.2  2.2.2  PHP Doctrine Collections library that adds additional functionality on top of PHP arrays.
doctrine/common                     3.4.4  3.4.4  PHP Doctrine Common project is a library that provides additional functionality that other Doctrine projects depend on such as better reflection support, proxie...
doctrine/data-fixtures              1.7.0  1.7.0  Data Fixtures for all Doctrine Object Managers
doctrine/deprecations               1.1.3  1.1.3  A small layer on top of trigger_error(E_USER_DEPRECATED) or PSR-3 logging with options to disable all deprecations or selectively for packages.
doctrine/event-manager              2.0.1  2.0.1  The Doctrine Event Manager is a simple PHP event system that was built to be used with the various Doctrine projects.
doctrine/inflector                  2.0.10 2.0.10 PHP Doctrine Inflector is a small library that can perform string manipulations with regard to upper/lowercase and singular/plural forms of words.
doctrine/instantiator               2.0.0  2.0.0  A small, lightweight utility to instantiate objects in PHP without invoking their constructors
doctrine/lexer                      3.0.1  3.0.1  PHP Doctrine Lexer parser library that can be used in Top-Down, Recursive Descent Parsers.
doctrine/sql-formatter              1.4.0  1.4.0  a PHP SQL highlighting library

PHP version

$ php -v
PHP 8.3.7 (cli) (built: May 14 2024 03:35:46) (ZTS)
Copyright (c) The PHP Group
Zend Engine v4.3.7, Copyright (c) Zend Technologies
    with Xdebug v3.3.2, Copyright (c) 2002-2024, by Derick Rethans

Subject

when using franken-php and Gedmo\Blameable it will try to create a new user instead of just setting setting the existing relation.

I fixed this with a simple Decorator, its a bit of a ugly solution, but works meanwhile the bug exists:

// @phpstan-ignore-next-line class.extendsFinalByPhpDoc
#[AsDecorator('stof_doctrine_extensions.listener.blameable')]
final class FrankenPHPBlameableListener extends BlameableListener
{
    public function __construct(
        private readonly ManagerRegistry $managerRegistry,
    ) {
        parent::__construct();
    }

    public function getFieldValue($meta, $field, $eventAdapter)
    {
        /** @var User $user */
        $user = parent::getFieldValue($meta, $field, $eventAdapter);
        // to prevent the user from being detached from the entity
        // while uploading a file in the rich-text-editor, it would try to create a new user entity
        // this started happening after using FrankenPHP
        return $this->managerRegistry->getRepository(User::class)->find($user->getId());
    }
}

Steps to reproduce

  • Frankenphp
  • Gedmo\Blameable
  • Create few entities and check the User table, there will be duplicates or it will fail for an unique constraint.

Expected results

It should not create a new user

Actual results

It creates a new user

@mbabker
Copy link
Contributor

mbabker commented Jul 17, 2024

TBH what you've got there is really the best solution for this without rewriting the blameable listener. Remember most of this package's code predates most long running process related code or architecture like FrankenPHP so running into issues like that isn't totally unexpected.

The only sane way I can think to improve this in the library is to support some kind of provider service for this (and other listeners) and then the application would be able to reset that service as appropriate for their setup (not using FrankenPHP myself but I imagine Symfony's ResetInterface would be used a bit here). Otherwise what you've got now where it has the extra application info (knowing you've pushed an entity into the listener, knowing what entity you're using, and a way to re-query it to deal with this specific issue) is really the nicest way to address it, even if it feels hacky.

@tuxes3
Copy link
Author

tuxes3 commented Jul 24, 2024

Thanks for reassuring that I am on the right path 😃

Might be worth to document this case, as long running php processes are more and more common. Furthermore to make it future proof there needs to be an interface for all the listeners. As you can see I had to extend from BlameableListener which will not work in the future (keyword final). Reason why a Decorator is not sufficient is because other Libraries are using this. For example in my case: https://github.com/stof/StofDoctrineExtensionsBundle/blob/main/src/EventListener/BlameListener.php#L23

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

No branches or pull requests

2 participants