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] Remove FixturesTrait #26

Merged
merged 47 commits into from
Mar 7, 2021
Merged

[2.x] Remove FixturesTrait #26

merged 47 commits into from
Mar 7, 2021

Conversation

alexislefebvre
Copy link
Collaborator

@alexislefebvre alexislefebvre commented Jul 8, 2019

See #18

TODO:

  • check that all the documentation is up-to-date

@alexislefebvre alexislefebvre changed the title Remove get container [WIP] Deprecate FixturesTrait Jul 8, 2019
@scaytrase
Copy link

#25 is done. any other show-stoppers?

@alexislefebvre
Copy link
Collaborator Author

I have to fix the tests.

zalas/phpunit-injector requires PHP 7.2+, so we need a solution to keep PHP 7.1 support with tests.

@scaytrase
Copy link

PHP 7.1 living out its days with security fixes only now. EOL is on Dec 2019. Is it really worth to support?

@alexislefebvre
Copy link
Collaborator Author

It may fix the performance issue reported in #12

@alexislefebvre
Copy link
Collaborator Author

Upgrade guide is missing.

@alexislefebvre
Copy link
Collaborator Author

Using zalas/phpunit-injector requires 2 new class dependencies, 2 new properties and their declaration.

Maybe we could keep the FixturesTrait in ordet to factorize this code.

doc/database.md Outdated Show resolved Hide resolved
doc/database.md Outdated Show resolved Hide resolved
doc/database.md Outdated Show resolved Hide resolved
@alexislefebvre alexislefebvre added the help wanted Extra attention is needed label Mar 3, 2021
@alexislefebvre alexislefebvre marked this pull request as ready for review March 3, 2021 21:53
doc/database.md Outdated Show resolved Hide resolved

$executor->getReferenceRepository()->save($this->getBackupFilePath());
self::$metadata = $em->getMetadataFactory()->getLoadedMetadata();

exec("mysqldump --host {$dbHost} {$port} {$dbPass} --user {$dbUser} --no-create-info --skip-triggers --no-create-db --no-tablespaces --compact {$dbName} > {$this->getBackupFilePath()}");
exec("{$dbPass} mysqldump --host {$dbHost} {$port} --user {$dbUser} --no-create-info --skip-triggers --no-create-db --no-tablespaces --compact {$dbName} > {$this->getBackupFilePath()}");
Copy link
Contributor

Choose a reason for hiding this comment

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

was this intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes the password is passed as an environment variable to avoid a warning from mysqldump.

@@ -77,12 +77,10 @@ public function loadFixtures(array $classNames = [], bool $append = false): Abst
if ($backupService) {
$event = new ReferenceSaveEvent($this->om, $executor, $backupService->getBackupFilePath());
$this->eventDispatcher->dispatch($event, LiipTestFixturesEvents::PRE_REFERENCE_SAVE);
$this->testCase->preReferenceSave($this->om, $executor, $backupService->getBackupFilePath());
Copy link
Contributor

Choose a reason for hiding this comment

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

is this already documented in the upgrading guide?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in #105

$eventDispatcher->addListener(
$eventName,
[$mock, $methodName]
);

// By loading fixtures, the events will be called (or not)
$fixtures = $this->loadFixtures([]);
if ($withCache) {
Copy link
Contributor

Choose a reason for hiding this comment

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

just FYI .. in such an if/else situation with single line assignments to the same variable, I personally prefer a ternary for readability. but I have no intention of enforcing such a personal preference

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 just read over the PR, I think overall this looks awesome. I will also ask for feedback via twitter

@alexislefebvre alexislefebvre force-pushed the remove-getContainer branch 4 times, most recently from d1db9ed to 328a74a Compare March 7, 2021 16:12
@alexislefebvre
Copy link
Collaborator Author

alexislefebvre commented Mar 7, 2021

Thanks for the review. I updated the documentation.

I will add events to the upgrade guide later. And update the services to use classes names too, right now it breaks the tests and I don't understand why.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants