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.

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

Jump to bottom

fix EventArgs::getEntityManger deprecation #2639

Merged
merged 6 commits into from
Oct 29, 2023
Merged

fix EventArgs::getEntityManger deprecation #2639

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4b45e37
use EventArgs::getObjectManager (fix DoctrineBundle 2.11 deprecation)
DubbleClick Aug 19, 2023
e48c9ca
add deprecation messages to test
DubbleClick Sep 10, 2023
4e3ff09
fix deprecation error type
DubbleClick Sep 18, 2023
c8ed0b2
add changelog note
DubbleClick Sep 18, 2023
7c50aab
clear up deprecation messages
DubbleClick Oct 29, 2023
fc2518a
re-add type to depreaction message
DubbleClick Oct 29, 2023
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
24 changes: 24 additions & 0 deletions src/Mapping/Event/Adapter/ORM.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,18 @@
throw new \LogicException(sprintf('Event args must be set before calling "%s()".', __METHOD__));
}

if (\method_exists($this->args, 'getObjectManager')) {
DubbleClick marked this conversation as resolved.
Show resolved Hide resolved
return $this->args->getObjectManager();
}

@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),
\TypeError::class
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be \Error::class?

Copy link
Contributor Author

@DubbleClick DubbleClick Sep 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think a TypeError is fine if we look at it as the $args class (type) not implementing the getObjectManager method. If we look at it like "the type $args::class must implement getObjectManager for version 4.0" then it should be \Error.

Edit: never mind, you're right. It wouldn't throw a TypeError.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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()

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we maybe add instanceof checks with LifecycleEventArgs, LoadClassMetadataEventArgs, ManagerEventArgs and OnClearEventArgs? Maybe they should all extend from ManagerEventArgs in doctrine/persistence 🤔 ? For the getObjectManager() call I mean, or any other solution?

It would be nice to finally move this forward.

Copy link
Contributor Author

@DubbleClick DubbleClick Oct 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't see any need to do that. The \method_exists call will make sure that it's fully backwards compatible for now.

If anything, that would be a discussion for later (when Doctrine ORM 3.0 releases and getEntityManager is removed
and we release DoctrineExtensions 4.0) when we remove the \method_exists call and directly call getObjectManager(). But even then, I don't see a point in checking the type of the args - all we care about is that they abide by Doctrines interfaces and implement getObjectManager() instead of getEntityManager() (that we currently also call without checking).

), E_USER_DEPRECATED);

Check warning on line 108 in src/Mapping/Event/Adapter/ORM.php

View check run for this annotation

Codecov / codecov/patch

src/Mapping/Event/Adapter/ORM.php#L102-L108 

Added lines #L102 - L108 were not covered by tests

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

Expand All @@ -104,6 +116,18 @@
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),
\TypeError::class
), E_USER_DEPRECATED);

Check warning on line 129 in src/Mapping/Event/Adapter/ORM.php

View check run for this annotation

Codecov / codecov/patch

src/Mapping/Event/Adapter/ORM.php#L123-L129 

Added lines #L123 - L129 were not covered by tests

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

Expand Down
31 changes: 31 additions & 0 deletions src/Uploadable/Event/UploadableBaseEventArgs.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

use Doctrine\Common\EventArgs;
use Doctrine\ORM\EntityManagerInterface;
use Doctrine\Persistence\ObjectManager;
use Gedmo\Uploadable\FileInfo\FileInfoInterface;
use Gedmo\Uploadable\UploadableListener;

Expand Down Expand Up @@ -100,6 +101,21 @@
* @return EntityManagerInterface
*/
public function getEntityManager()
{
@trigger_error(sprintf(
'"%s()" is deprecated since gedmo/doctrine-extensions 3.x and will be removed in version 4.0.',
__METHOD__
), E_USER_DEPRECATED);

Check warning on line 108 in src/Uploadable/Event/UploadableBaseEventArgs.php

View check run for this annotation

Codecov / codecov/patch

src/Uploadable/Event/UploadableBaseEventArgs.php#L105-L108 

Added lines #L105 - L108 were not covered by tests

return $this->em;

Check warning on line 110 in src/Uploadable/Event/UploadableBaseEventArgs.php

View check run for this annotation

Codecov / codecov/patch

src/Uploadable/Event/UploadableBaseEventArgs.php#L110 

Added line #L110 was not covered by tests
}

/**
* Retrieve associated EntityManager
*
* @return ObjectManager
*/
public function getObjectManager()

Check warning on line 118 in src/Uploadable/Event/UploadableBaseEventArgs.php

View check run for this annotation

Codecov / codecov/patch

src/Uploadable/Event/UploadableBaseEventArgs.php#L118 

Added line #L118 was not covered by tests
{
return $this->em;
DubbleClick marked this conversation as resolved.
Show resolved Hide resolved
}
Expand All @@ -110,6 +126,21 @@
* @return object
*/
public function getEntity()
{
@trigger_error(sprintf(
'"%s()" is deprecated since gedmo/doctrine-extensions 3.x and will be removed in version 4.0.',
__METHOD__
), E_USER_DEPRECATED);

Check warning on line 133 in src/Uploadable/Event/UploadableBaseEventArgs.php

View check run for this annotation

Codecov / codecov/patch

src/Uploadable/Event/UploadableBaseEventArgs.php#L130-L133 

Added lines #L130 - L133 were not covered by tests

return $this->entity;

Check warning on line 135 in src/Uploadable/Event/UploadableBaseEventArgs.php

View check run for this annotation

Codecov / codecov/patch

src/Uploadable/Event/UploadableBaseEventArgs.php#L135 

Added line #L135 was not covered by tests
}

/**
* Retrieve associated Object
*
* @return object
*/
public function getObject()

Check warning on line 143 in src/Uploadable/Event/UploadableBaseEventArgs.php

View check run for this annotation

Codecov / codecov/patch

src/Uploadable/Event/UploadableBaseEventArgs.php#L143 

Added line #L143 was not covered by tests
{
return $this->entity;
}
Expand Down
4 changes: 2 additions & 2 deletions tests/Gedmo/Mapping/MappingEventAdapterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,10 @@ public function testAdapterBehavior(): void
->disableOriginalConstructor()
->getMock();
$eventArgsMock->expects(static::once())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we could even replace these mocks to use a class that is no deprecated, like Doctrine\ORM\Event\PrePersistEventArgs or Doctrine\ORM\Event\PreUpdateEventArgs.

->method('getEntityManager');
->method('getObjectManager');

$eventArgsMock->expects(static::once())
->method('getEntity')
->method('getObject')
->willReturn(new \stdClass());

$eventAdapter = new EventAdapterORM();
Expand Down
Loading