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

[2.x] Add events #81

Merged
merged 11 commits into from
Feb 22, 2021
Merged

[2.x] Add events #81

merged 11 commits into from
Feb 22, 2021

Conversation

alexislefebvre
Copy link
Collaborator

@alexislefebvre alexislefebvre commented Jan 7, 2021

I took inspiration from FOSUserBundle: https://github.com/FriendsOfSymfony/FOSUserBundle/blob/8bb87890cbd872c870f7bd146922906f6e8ddf5c/FOSUserEvents.php#L26

TODO:

  • Remove ObjectManager, ReferenceRepository and AbstractExecutor from the events? Users could inject these services in their listeners
  • Declare a listener and set PHPUnit mock to ensure that the listeners are called
  • Restore the ReferenceRepository in the relevant event, as requested in [RFC] Post Fixtures Database restore #27?
  • Move symfony/event-dispatcher and symfony/event-dispatcher-contracts in require instead of require-dev

@alexislefebvre alexislefebvre self-assigned this Jan 7, 2021
@alexislefebvre alexislefebvre added help wanted Extra attention is needed enhancement New feature or request labels Jan 7, 2021
@alexislefebvre alexislefebvre marked this pull request as draft January 8, 2021 21:15
@alexislefebvre alexislefebvre changed the title [WIP] Add events Add events Jan 8, 2021
@alexislefebvre alexislefebvre force-pushed the add-events branch 2 times, most recently from 6483024 to d140253 Compare January 8, 2021 22:40
@alexislefebvre alexislefebvre marked this pull request as ready for review January 11, 2021 12:15
@lsmith77
Copy link
Contributor

I guess this would be the basis for a 2.x release?

@lsmith77
Copy link
Contributor

@stof since you seem to have authored similar changes in FOSUserBundle, can you also have a look here?

@alexislefebvre
Copy link
Collaborator Author

alexislefebvre commented Jan 12, 2021

@lsmith77

I guess this would be the basis for a 2.x release?

This PR doesn't add any BC break, so this can be merged as is. But we will have duplicated lines about events:

$event = new FixtureBackupEvent($backupService->getBackupFilePath());
$this->dispatchEvent($event, LiipTestFixturesEvents::PRE_FIXTURE_BACKUP_RESTORE);
$this->testCase->preFixtureBackupRestore($this->om, $referenceRepository, $backupService->getBackupFilePath());

Then we could remove $this->testCase->preFixtureBackupRestore(…) and similar calls for the 2.x version.

I would like to implement #26 in order to provide a 2.x version that work without Trait too.

@@ -221,4 +230,22 @@ protected function getCacheMetadataParameter()
return $this->container->hasParameter(self::CACHE_METADATA_PARAMETER_NAME)
&& false !== $this->container->getParameter(self::CACHE_METADATA_PARAMETER_NAME);
}

/**
* Compatibility layer for Symfony <= 4.3
Copy link
Contributor

Choose a reason for hiding this comment

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

do we even want to still care about <- 4.3? https://symfony.com/releases

Copy link
Contributor

Choose a reason for hiding this comment

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

or would not including this create a BC break?

Copy link

Choose a reason for hiding this comment

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

well, currently, the composer.json says it supports Symfony 3.4+.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would prefer to support Symfony 3.4 as long as it doesn't cause too many trouble.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, fine for me!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Finally, I changed my mind and removed support of Symfony < 4.4. Symfony 3.4 will only get security fixes now, I think no one would upgrade that bundle while keeping a legacy version of Symfony.

@lsmith77
Copy link
Contributor

lsmith77 commented Jan 19, 2021 via email

Copy link
Contributor

@lsmith77 lsmith77 left a comment

Choose a reason for hiding this comment

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

I suggest we create a new 2.x branch and point this PR at 2.x

wdyt?

src/Event/PostFixtureBackupRestoreEvent.php Outdated Show resolved Hide resolved
@alexislefebvre alexislefebvre changed the base branch from master to 2.x February 20, 2021 13:24
@lsmith77
Copy link
Contributor

LGTM aside from the small CS issue I noticed

@alexislefebvre
Copy link
Collaborator Author

Thanks for the review @lsmith77 ! I fixed the issue and will merge once CI is finished.

@alexislefebvre alexislefebvre merged commit da57b0b into 2.x Feb 22, 2021
@alexislefebvre alexislefebvre deleted the add-events branch February 22, 2021 14:43
@alexislefebvre alexislefebvre changed the title Add events [2.x] Add events Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants