fix EventArgs::getEntityManger deprecation #2639

DubbleClick · 2023-06-28T13:39:27Z

doc/symfony6.md

src/Uploadable/Event/UploadableBaseEventArgs.php

DubbleClick · 2023-07-02T03:59:33Z

@franmomu please trigger the workflows again, getEntity() should no longer be called by us.

DubbleClick · 2023-07-02T04:19:13Z

It is however noteworthy that there is use of plenty other deprecated functionality throughout the project (EntityManager::create) for example. Not all of it is trivial to work around and that unfortunately exceeds the time I can currently invest.

phansys · 2023-07-03T15:20:26Z

doc/symfony6.md

@@ -0,0 +1,445 @@
+# Install Gedmo Doctrine extensions in Symfony 6


How is the integration with Symfony 6 related to the deprecation fix for EventArgs::getEntityManger()?

It's not related to EntityManager, it's related to the deprecation of Event Subscribers. This comes from the Symfony Bridge: symfony/symfony#49586

The simpler solution here might be to make the documentation link to the bundle at https://packagist.org/packages/stof/doctrine-extensions-bundle, which handles registering listeners instead of subscribers, with CI checks warning us if the events used by a listener are updated.

franmomu · 2023-07-25T09:21:07Z

I think we are doing two things here, apart from avoiding calls to getEntityManager() from doctrine, we are also deprecating some of our methods, I would leave deprecating these methods for another PR (which would require a minor release)

DubbleClick · 2023-07-25T13:01:40Z

I'll be happy to remove the deprecation of the methods and open another PR for that. Should I?

franmomu · 2023-07-26T17:19:56Z

I was thinking about adding some fixes like this one and #2651 at least and make a patch release and then make a minor one adding deprecations + removing support for php < 7.4 and probably symfony < 5.4 cc @phansys

DubbleClick · 2023-07-27T09:35:40Z

and #2634 for the holy trinity of pull requests to get rid of all deprecations for now

franmomu · 2023-08-14T13:04:15Z

@DubbleClick if you could please split this PR into two, one that fixes triggering deprecations (could be this one) and another one deprecating methods.

The symfony6.md file won't be necessary, we'll deal with that in #2667 (thanks for creating the file).

DubbleClick · 2023-08-14T13:08:58Z

yep will do it tomorrow

DubbleClick · 2023-08-17T13:38:08Z

@franmomu @phansys sorry, took a little longer

I removed all the deprecations and only kept in the added methods (and a safety check for $platform)

DubbleClick · 2023-08-17T13:59:27Z

rebased it all to only show up in one commit and also created one for EntityManager::create deprecations at #2669

DubbleClick · 2023-08-18T12:10:22Z

I'm not entirely sure how to get rid of all the warnings. Either there will be a warning that ObjectManager is returned rather than EntityManagerInterface, or there will be warnings that ObjectManager doesn't have methods like createQueryBuilder.

src/Uploadable/Event/UploadableBaseEventArgs.php

franmomu

Thanks for keep working on this, sorry about reverting changes, but it's better if in the same commit there are only changes related to the current fix.

Sorry that I wasn't clear 😞, what I meant in #2639 (comment) was to only add the necessary code to not trigger deprecations.

I think adding ORM::setObjectManager, UploadableBaseEventArgs::getObjectManager() and UploadableBaseEventArgs::getObject() makes sense if we deprecate the old ones, so I would leave that to the other PR.

src/SoftDeleteable/Filter/SoftDeleteableFilter.php

src/Translatable/Query/TreeWalker/TranslationWalker.php

DubbleClick · 2023-08-19T10:55:06Z

Sorry that I wasn't clear 😞, what I meant in #2639 (comment) was to only add the necessary code to not trigger deprecations.

No worries, I've removed everything not strictly necessary in order to not trigger deprecations now.

I think adding ORM::setObjectManager, UploadableBaseEventArgs::getObjectManager() and UploadableBaseEventArgs::getObject() makes sense if we deprecate the old ones, so I would leave that to the other PR.

It'll be inevitable at some point, but it doesn't have to happen right now.

franmomu · 2023-08-19T16:54:29Z

tests/Gedmo/Mapping/MappingEventAdapterTest.php

@@ -12,7 +12,7 @@
 namespace Gedmo\Tests\Mapping;

 use Doctrine\ORM\EntityManager;
-use Doctrine\ORM\Event\LifecycleEventArgs;
+use Doctrine\Persistence\Event\LifecycleEventArgs;


There is this code 😞

DoctrineExtensions/src/Mapping/MappedEventSubscriber.php

Lines 252 to 277 in 351caed

/**

* Get an event adapter to handle event specific

* methods

*

* @throws InvalidArgumentException if event is not recognized

*

* @return AdapterInterface

*/

protected function getEventAdapter(EventArgs $args)

{

$class = get_class($args);

if (preg_match('@Doctrine\\\([^\\\]+)@', $class, $m) && in_array($m[1], ['ODM', 'ORM'], true)) {

if (!isset($this->adapters[$m[1]])) {

$adapterClass = $this->getNamespace().'\\Mapping\\Event\\Adapter\\'.$m[1];

if (!class_exists($adapterClass)) {

$adapterClass = 'Gedmo\\Mapping\\Event\\Adapter\\'.$m[1];

}

$this->adapters[$m[1]] = new $adapterClass();

}

$this->adapters[$m[1]]->setEventArgs($args);

return $this->adapters[$m[1]];

}

throw new InvalidArgumentException('Event mapper does not support event arg class: '.$class);

}

that do some funny things to determine which event adapter has to be instantiated and it's based on the event class namespace that should include ORM or ODM.

It would be awesome to be able to get rid of this code, but since it doesn't seem easy, maybe we should keep using the deprecated Doctrine\Persistence\Event\LifecycleEventArgs;, once #2669 is merged we can use one of the new dedicated events.

GitHub doesn't have the correct emoji to express my feelings about that code. Sometimes I feel like code reflection goes just a little too far in some languages, hahaha.

Anyway, I've reverted back to Doctrine\ORM\Event\LifecycleEventArgs.

Regarding getEventAdapter(), I stumbled with that yesterday in my tests 😥

GitHub doesn't have the correct emoji to express my feelings about that code. Sometimes I feel like code reflection goes just a little too far in some languages, hahaha.

😵 😵‍💫 🤣

It would be awesome to be able to get rid of this code, but since it doesn't seem easy, maybe we should keep using the deprecated Doctrine\Persistence\Event\LifecycleEventArgs;, once #2669 is merged we can use one of the new dedicated events.

Just for the records, Doctrine\ORM\Event\LifecycleEventArgs is deprecated, but Doctrine\Persistence\Event\LifecycleEventArgs is not.

codecov · 2023-08-20T12:05:43Z

Codecov Report

Attention: 26 lines in your changes are missing coverage. Please review.

Comparison is base (23316d6) 79.64% compared to head (fc2518a) 79.41%.
Report is 8 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2639      +/-   ##
==========================================
- Coverage   79.64%   79.41%   -0.23%     
==========================================
  Files         161      162       +1     
  Lines        8468     8482      +14     
==========================================
- Hits         6744     6736       -8     
- Misses       1724     1746      +22

Files	Coverage Δ
src/Uploadable/Event/UploadableBaseEventArgs.php	`0.00% <0.00%> (ø)`
src/Mapping/Event/Adapter/ORM.php	`50.74% <22.22%> (-14.56%)`	⬇️

... and 27 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

franmomu

I see this fine after adding the changelog, I'll approve since I'm not sure if this week I'll be available

src/Mapping/Event/Adapter/ORM.php

CHANGELOG.md

phansys · 2023-09-18T13:57:34Z

src/Mapping/Event/Adapter/ORM.php

+            .' and will throw a "%s" error in version 4.0.',
+            __METHOD__,
+            get_class($this->args),
+            \TypeError::class


If there won't be an instanceof check here, I guess we can't even be sure if\Error will be thrown. If that is the case, I guess we should say something like "it will be not supported" instead.

raziel057 · 2023-09-27T14:20:14Z

Hi, just for information I switched to the https://github.com/DubbleClick/DoctrineExtensions/commits/main branch on my project and works as expected. I still get deprecation related to the use of Doctrine\Persistence\Event\LifecycleEventArgs. That should be fixed by merge of the following PR: #2649

DubbleClick · 2023-09-27T14:44:06Z

@phansys "If there won't be an instanceof check here, I guess we can't even be sure if\Error will be thrown. If that is the case, I guess we should say something like "it will be not supported" instead."

No instanceof check is required, calling a method that doesn't exist throws an \Error.

Fatal error: Uncaught Error: Call to undefined method OldEventArgs::getObjectManager()

@raziel057 thanks for the confirmation, but be warned that the fork will likely be deleted the day this is merged :p

raziel057 · 2023-09-27T14:54:48Z

@DubbleClick No problem thanks we will switch back as soon as this is merged.

phansys · 2023-09-27T19:14:14Z

@phansys "If there won't be an instanceof check here, I guess we can't even be sure if\Error will be thrown. If that is the case, I guess we should say something like "it will be not supported" instead."
No instanceof check is required, calling a method that doesn't exist throws an \Error.

What is strange (and wrong) at first glance to me, is that we will call a method that is not guaranteed by the object we are trying to call it from. In other words, we are allowing a method call without confirming whether that is possible.
I'd like to understand what you think the method's body will be like in the next major release, because I guess we should avoid calling a non-existent method, preventing the call with our own mechanism in the worst case if we can't trust the contract provided by the class or interface.

DubbleClick · 2023-09-27T20:17:29Z

What is strange (and wrong) at first glance to me, is that we will call a method that is not guaranteed by the object we are trying to call it from. In other words, we are allowing a method call without confirming whether that is possible. I'd like to understand what you think the method's body will be like in the next major release, because I guess we should avoid calling a non-existent method, preventing the call with our own mechanism in the worst case if we can't trust the contract provided by the class or interface.

if (\method_exists($this->args, 'getObject')) {
    return $this->args->getObject();
}

@trigger_error(sprintf(
    'Calling "%s()" on an event of type "%s" is deprecated since gedmo/doctrine-extensions 3.x'
    .' and will throw a "%s" error in version 4.0.',
    __METHOD__,
    get_class($this->args),
    \Error::class
), E_USER_DEPRECATED);

return $this->args->getEntity();

becomes

return $this->args->getObject();

If the method doesn't exist, PHP throws an \Error.
Just like Doctrine will result in the same error when you attempt to call getEntity() on their EventArgs next release.

phansys · 2023-09-27T22:30:02Z

If the method doesn't exist, PHP throws an \Error.
Just like Doctrine will result in the same error when you attempt to call getEntity() on their EventArgs next release.

Thanks for the clarification.
I don't feel comfortable admitting an internal PHP error (\Error) when we already know this problem is part of a design flaw on our side.

DubbleClick · 2023-09-28T07:02:10Z

If the method doesn't exist, PHP throws an \Error.
Just like Doctrine will result in the same error when you attempt to call getEntity() on their EventArgs next release.

Thanks for the clarification. I don't feel comfortable admitting an internal PHP error (\Error) when we already know this problem is part of a design flaw on our side.

I don't think it's a design flaw - doctrine changed its functionality to deprecate getEntity() and replace it by getObject(), so we react to it and call getObject(). For the current version we keep getEntity() as a fallback for people using their own EventArgs implementations, but deprecate it. In the next major version, following Doctrine, we can expect people to have refactored their own EventArgs.

franmomu · 2023-10-24T13:44:43Z

If the method doesn't exist, PHP throws an \Error.
Just like Doctrine will result in the same error when you attempt to call getEntity() on their EventArgs next release.

Thanks for the clarification. I don't feel comfortable admitting an internal PHP error (\Error) when we already know this problem is part of a design flaw on our side.

@phansys so what do you think we should do here, it would be nice to finally finish and merge this PR.

phansys · 2023-10-29T11:55:19Z

@phansys so what do you think we should do here, it would be nice to finally finish and merge this PR.

First of all, sorry for the delay on the feedback.

Currently, the PR has some deprecation messages that indicate what will be disallowed in the next major release, but IMO, there is no clear path about how the restriction will be implemented.

    public function getObject(): object
    {
        if (null === $this->args) {
            throw new \LogicException(sprintf('Event args must be set before calling "%s()".', __METHOD__));
        }

        if (\method_exists($this->args, 'getObject')) {
            return $this->args->getObject();
        }

        @trigger_error(sprintf(
            'Calling "%s()" on an event of type "%s" is deprecated since gedmo/doctrine-extensions 3.x'
            .' and will throw a "%s" error in version 4.0.',
            __METHOD__,
            get_class($this->args),
            \Error::class
        ), E_USER_DEPRECATED);

        return $this->args->getEntity();
    }

By instance, I don't know if the call to method_exists() will be removed because we should be trusting in the type of $this->args based on a previous type-safe check, or if we are proposing to let the PHP engine to reach a condition where we call a non-existent method getObject() which can be not defining depending on the implementation of $this->args.

I think a comment with the code that must replace these deprecations should be enough.
I'm thinking of a kind of instruction like this:

DoctrineExtensions/src/Mapping/Driver/Chain.php

Lines 90 to 92 in aaa8c15

    
           // @todo: In the next major release remove the assignment to `$extendedMetadata`, the previous conditional 
        
           // block, uncomment the following line and replace the following return statement. 
        
           // return $driver->readExtendedMetadata($meta, $config);

DubbleClick · 2023-10-29T12:10:38Z

By instance, I don't know if the call to method_exists() will be removed because we should be trusting in the type of $this->args based on a previous type-safe check, or if we are proposing to let the PHP engine to reach a condition where we call a non-existent method getObject() which can be not defining depending on the implementation of $this->args.

Isn't that necessarily the same behaviour as it is now? There's even less of a guarantee that the EventArgs implement getEntityManager than there is that they implement getObjectManager.

I'm happy to write a todo block for the next major release, though.

phansys · 2023-10-29T12:18:31Z

Isn't that necessarily the same behaviour as it is now? There's even less of a guarantee that the EventArgs implement getEntityManager than there is that they implement getObjectManager.

If this is the case, I'd choose a path that leaves under our own control the responsibility to throw an error explaining in a clear way why the configured $this->args is not valid for the context where it is used.

and add instructions for the next major release

DubbleClick · 2023-10-29T12:28:45Z

I added instructions and cleared up the error message a little better.

Honestly, I am not even sure how far out of their way people would have to go to have DoctrineExtensions catch their own EventArgs implementations. All the Doctrine events we catch and our own implement getObjectManager.

franmomu · 2023-10-29T12:52:32Z

Thanks @DubbleClick ❤️

brzuchal · 2023-11-17T07:47:43Z

When to expect a release of this?

DubbleClick · 2023-11-17T08:34:15Z

in all likelihood before orm 3.0, so I wouldn't worry about it :)

franmomu · 2023-11-19T16:13:31Z

I hope to release this soon, but I would like to include in the release also these PR:
#2715
#2716
#2720
#2556

* use EventArgs::getObjectManager (fix DoctrineBundle 2.11 deprecation) * add deprecation messages to test * fix deprecation error type * add changelog note * clear up deprecation messages and add instructions for the next major release * re-add type to depreaction message

DubbleClick commented Jun 28, 2023

View reviewed changes

doc/symfony6.md Outdated Show resolved Hide resolved

src/Uploadable/Event/UploadableBaseEventArgs.php Show resolved Hide resolved

phansys reviewed Jul 3, 2023

View reviewed changes

phansys requested a review from franmomu July 24, 2023 23:04

DubbleClick mentioned this pull request Aug 17, 2023

Remplace getEntityManager by getObjectManager to remove deprecation message symfony 6.3 (#2664) #2665

Closed

franmomu reviewed Aug 19, 2023

View reviewed changes

src/Uploadable/Event/UploadableBaseEventArgs.php Outdated Show resolved Hide resolved

src/Uploadable/Event/UploadableBaseEventArgs.php Outdated Show resolved Hide resolved

src/Uploadable/Event/UploadableBaseEventArgs.php Outdated Show resolved Hide resolved

franmomu reviewed Aug 19, 2023

View reviewed changes

src/SoftDeleteable/Filter/SoftDeleteableFilter.php Outdated Show resolved Hide resolved

src/Translatable/Query/TreeWalker/TranslationWalker.php Outdated Show resolved Hide resolved

src/Translatable/Query/TreeWalker/TranslationWalker.php Outdated Show resolved Hide resolved

franmomu reviewed Aug 19, 2023

View reviewed changes

franmomu added the Needs Changelog note label Aug 21, 2023

franmomu approved these changes Aug 21, 2023

View reviewed changes

phansys reviewed Aug 21, 2023

View reviewed changes

src/Mapping/Event/Adapter/ORM.php Show resolved Hide resolved

franmomu mentioned this pull request Sep 8, 2023

fix EntityManager::create deprecations #2669

Merged

phansys added the Needs Rebase label Sep 8, 2023

franmomu removed Needs Changelog note Needs Rebase labels Sep 8, 2023

franmomu requested a review from phansys September 8, 2023 14:23

phansys reviewed Sep 9, 2023

View reviewed changes

src/Mapping/Event/Adapter/ORM.php Outdated Show resolved Hide resolved

use EventArgs::getObjectManager (fix DoctrineBundle 2.11 deprecation)

4b45e37

franmomu requested a review from phansys September 18, 2023 13:39

phansys reviewed Sep 18, 2023

View reviewed changes

add changelog note

c8ed0b2

franmomu removed the Needs Changelog note label Oct 17, 2023

mbabker mentioned this pull request Oct 17, 2023

Refactor ORM QueryBuilder::setParameters() calls to use setParameter() instead due to a stricter type in 3.0 #2710

Merged

clear up deprecation messages

7c50aab

and add instructions for the next major release

re-add type to depreaction message

fc2518a

phansys approved these changes Oct 29, 2023

View reviewed changes

franmomu merged commit 7a7612a into doctrine-extensions:main Oct 29, 2023
19 of 21 checks passed

This was referenced Oct 29, 2023

Doctrine deprecations #2638

Closed

EventArgs::getEntityManager is replaced by getObjectManager in Doctrine parent classes #2640

Closed

Use real event for test #2718

Merged

misaert mentioned this pull request Feb 4, 2024

Deprecation message symfony 6.3 #2664

Closed

		@@ -0,0 +1,445 @@
		# Install Gedmo Doctrine extensions in Symfony 6

	/**
	* Get an event adapter to handle event specific
	* methods
	*
	* @throws InvalidArgumentException if event is not recognized
	*
	* @return AdapterInterface
	*/
	protected function getEventAdapter(EventArgs $args)
	{
	$class = get_class($args);
	if (preg_match('@Doctrine\\\([^\\\]+)@', $class, $m) && in_array($m[1], ['ODM', 'ORM'], true)) {
	if (!isset($this->adapters[$m[1]])) {
	$adapterClass = $this->getNamespace().'\\Mapping\\Event\\Adapter\\'.$m[1];
	if (!class_exists($adapterClass)) {
	$adapterClass = 'Gedmo\\Mapping\\Event\\Adapter\\'.$m[1];
	}
	$this->adapters[$m[1]] = new $adapterClass();
	}
	$this->adapters[$m[1]]->setEventArgs($args);

	return $this->adapters[$m[1]];
	}

	throw new InvalidArgumentException('Event mapper does not support event arg class: '.$class);
	}

fix EventArgs::getEntityManger deprecation #2639

fix EventArgs::getEntityManger deprecation #2639

Conversation

DubbleClick commented Jun 28, 2023 • edited Loading

DubbleClick commented Jul 2, 2023

DubbleClick commented Jul 2, 2023

phansys Jul 3, 2023

Choose a reason for hiding this comment

DubbleClick Jul 4, 2023

Choose a reason for hiding this comment

stof Aug 23, 2023

Choose a reason for hiding this comment

franmomu commented Jul 25, 2023

DubbleClick commented Jul 25, 2023

franmomu commented Jul 26, 2023

DubbleClick commented Jul 27, 2023

franmomu commented Aug 14, 2023

DubbleClick commented Aug 14, 2023

DubbleClick commented Aug 17, 2023

DubbleClick commented Aug 17, 2023

DubbleClick commented Aug 18, 2023

franmomu left a comment

Choose a reason for hiding this comment

DubbleClick commented Aug 19, 2023

franmomu Aug 19, 2023

Choose a reason for hiding this comment

DubbleClick Aug 20, 2023

Choose a reason for hiding this comment

phansys Sep 10, 2023

Choose a reason for hiding this comment

codecov bot commented Aug 20, 2023 • edited Loading

Codecov Report

franmomu left a comment

Choose a reason for hiding this comment

phansys Sep 18, 2023

Choose a reason for hiding this comment

raziel057 commented Sep 27, 2023

DubbleClick commented Sep 27, 2023

raziel057 commented Sep 27, 2023

phansys commented Sep 27, 2023

DubbleClick commented Sep 27, 2023 • edited Loading

phansys commented Sep 27, 2023

DubbleClick commented Sep 28, 2023

franmomu commented Oct 24, 2023

phansys commented Oct 29, 2023

DubbleClick commented Oct 29, 2023

phansys commented Oct 29, 2023

DubbleClick commented Oct 29, 2023

franmomu commented Oct 29, 2023

brzuchal commented Nov 17, 2023

DubbleClick commented Nov 17, 2023

franmomu commented Nov 19, 2023

DubbleClick commented Jun 28, 2023 •

edited

Loading

codecov bot commented Aug 20, 2023 •

edited

Loading

DubbleClick commented Sep 27, 2023 •

edited

Loading