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

Fix: ExecuteCommand #433

Merged
merged 3 commits into from
Feb 18, 2016

Conversation

rcatlin
Copy link
Contributor

@rcatlin rcatlin commented Feb 16, 2016

  • Fix \Doctrine\DBAL\Migrations\Tools\Console\Command\ExecuteCommand unable to find Migration Version by registering Migration Versions on call to \Doctrine\DBAL\Migrations\Configuration\Configuration#getVersion()
  • Adds missing check for empty migrations array in \Doctrine\DBAL\Migrations\Configuration\Configuration#getVersion()
  • Commit 916bee2 previously added checks for registered migrations (Pull Request Fix issue with some method when the migrations were not loaded #421)
  • Added tests for \Doctrine\DBAL\Migrations\Configuration\Configuration#getVersion()

Fix issue with some method call order when the migrations were not loaded

Previously you needed to make sure manually that registerMigrations was
called before to make any call to a method that was using the migrations
loaded.

This might cause performance issue in some edge case but it's unlikely.
Unfortunately in the current desgin I don't see any other solution.

@rcatlin
Copy link
Contributor Author

rcatlin commented Feb 16, 2016

@mikeSimonson review?

@mikeSimonson
Copy link
Contributor

@rcatlin Can you add a test for it ?

@rcatlin
Copy link
Contributor Author

rcatlin commented Feb 17, 2016

@mikeSimonson Absolutely!

@rcatlin
Copy link
Contributor Author

rcatlin commented Feb 17, 2016

@mikeSimonson Added tests for Configuration#getVersion(). Please take another look when you have a chance.

@mikeSimonson mikeSimonson self-assigned this Feb 17, 2016
@mikeSimonson
Copy link
Contributor

@rcatlin Thanks

mikeSimonson added a commit that referenced this pull request Feb 18, 2016
@mikeSimonson mikeSimonson merged commit 62acf77 into doctrine:master Feb 18, 2016
@mikeSimonson mikeSimonson added this to the 1.3.1 milestone Feb 18, 2016

public function testGetVersionNotFound()
{
$this->setExpectedException(MigrationException::class);
Copy link
Member

Choose a reason for hiding this comment

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

Putting this after the call to getSqliteConfiguration would be even better, as it would enforce the exception to happen in getVersion rather than getSqliteConfiguration (which is the drawback of the @expectedException annotation btw)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's an excellent point! I'll put up a PR with the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: Pull Request created #435

@rcatlin rcatlin deleted the fix/execute-command-registers branch February 18, 2016 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants