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

Allow doctrine/event-manager 2 #2480

Merged
merged 2 commits into from
Oct 31, 2022
Merged

Allow doctrine/event-manager 2 #2480

merged 2 commits into from
Oct 31, 2022

Conversation

franmomu
Copy link
Contributor

Q A
Type feature
BC Break no
Fixed issues

Summary

I haven't found any calls to EventManager::getListeners() method.

@franmomu franmomu added this to the 2.5.0 milestone Oct 30, 2022
@malarzm
Copy link
Member

malarzm commented Oct 30, 2022

@franmomu any opinion on hasListeners usage? I was thinking we could get rid of it #2478

@malarzm malarzm linked an issue Oct 30, 2022 that may be closed by this pull request
Support doctrine/event-manager 2.x #2478
Closed
@franmomu
Copy link
Contributor Author

hasListeners

I didn't see the issue open 🤦 yeah I'll take a look at those cases

@malarzm malarzm added Task and removed Feature labels Oct 31, 2022
@franmomu
Remove unnecessary check
a1ab62c 
EventManager::dispatchEvent already checks if there are listeners for the event.
@franmomu
Copy link
Contributor Author

There is this one left:

mongodb-odm/lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataFactory.php

Lines 90 to 101 in f5c1679

protected function onNotFoundMetadata($className)
{
if (! $this->evm->hasListeners(Events::onClassMetadataNotFound)) {
return null;
}
$eventArgs = new OnClassMetadataNotFoundEventArgs($className, $this->dm);
$this->evm->dispatchEvent(Events::onClassMetadataNotFound, $eventArgs);
return $eventArgs->getFoundMetadata();
}

which maybe it's easier to read with the check first, but we could change it too.

franmomu
franmomu commented Oct 31, 2022
View reviewed changes
lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataFactory.php
$this->evm->dispatchEvent(
Events::loadClassMetadata,
new LoadClassMetadataEventArgs($class, $this->dm)
);

// phpcs:ignore SlevomatCodingStandard.ControlStructures.EarlyExit.EarlyExitNotUsed
if ($class->isChangeTrackingNotify()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like it should have been called always not only when there are no event listeners for Events::loadClassMetadata.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch 👍

malarzm
malarzm approved these changes Oct 31, 2022
View reviewed changes
lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataFactory.php
$this->evm->dispatchEvent(
Events::loadClassMetadata,
new LoadClassMetadataEventArgs($class, $this->dm)
);

// phpcs:ignore SlevomatCodingStandard.ControlStructures.EarlyExit.EarlyExitNotUsed
if ($class->isChangeTrackingNotify()) {
Copy link
Member

Choose a reason for hiding this comment

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

Good catch 👍

@malarzm
Copy link
Member

malarzm commented Oct 31, 2022

Re onNotFoundMetadata method: yeah, this seems more readbale this way :) Let's keep it as is.

@malarzm malarzm merged commit a6c0f65 into doctrine:2.5.x Oct 31, 2022
@malarzm
Copy link
Member

malarzm commented Oct 31, 2022

Thanks @franmomu!

@franmomu franmomu deleted the event_manager branch October 31, 2022 20:14
@malarzm malarzm mentioned this pull request Oct 31, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@malarzm malarzm malarzm approved these changes

Assignees
No one assigned
Labels
Task
Projects
None yet
Milestone
2.5.0
Development

Successfully merging this pull request may close these issues.

Support doctrine/event-manager 2.x
2 participants
@franmomu @malarzm