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

Allow Migration Classes to be in Multiple Namespaces #693

Merged
merged 17 commits into from May 29, 2018

Conversation

Projects
None yet
2 participants
@chrisguitarguy
Copy link
Contributor

chrisguitarguy commented May 20, 2018

Q A
Type Improvement
BC Break yes
Fixed issues #348

Summary

This changes the various MigrationFinder implementations to use reflection and get_declared_classes to load the various migration classes. The code is similar to the doctrine persistence stuff

The end result is that all the code for loading migrations no longer cares about the namespace in the configuration -- it's only for generating new migrations now. Users can nest namespaces as they wish.

chrisguitarguy added some commits May 19, 2018

Use Reflection to Determine Version Number & FQCNs
Previously the finders would take whatever namespace was given in the
findMigrations method. Instead, this switches that to use reflection
to both figure out a version number (from the short class name) and the
FQCN as well.
Move Class Exists Test to MigrationRepository Test
With the new finder stuff that uses reflection instead of guessing
filenames, the test was failing and not really able to be fixed without
breaking the finder stuff.

Due to refactoring, it made more sense to move the test into
`MigrationRepositoryTest`.
Remove Unused `$namespace` Argument from `findMigrations`
The previous changes to use reflection and `get_declared_classes` means
that this argument is no longer used.

@chrisguitarguy chrisguitarguy force-pushed the chrisguitarguy:finder_ns branch from 11e115f to c4bb492 May 20, 2018

}
return $migrations;
ksort($versions, SORT_STRING);

This comment has been minimized.

@chrisguitarguy

chrisguitarguy May 20, 2018

Contributor

Do we need this sorting? The MigrationRepository does sorting itself. Maybe its redundant here?

This comment has been minimized.

@jwage

jwage May 21, 2018

Member

Probably not needed. I would make sure we have test coverage for the sorting and then remove it and see if anything breaks.

$classes = [];
foreach (get_declared_classes() as $class) {
$ref = new ReflectionClass($class);
if (in_array($ref->getFileName(), $files)) {

This comment has been minimized.

@chrisguitarguy

chrisguitarguy May 20, 2018

Contributor

PHP CS is complaining about this line on my local because it doesn't use an early exit.

MigrationException::class,
'Migration class "Migrations\Version123" was not found. Is it placed in "Migrations" namespace?'
);
$configuration->registerMigrationsFromDirectory($migrationsDir);

This comment has been minimized.

@chrisguitarguy

chrisguitarguy May 20, 2018

Contributor

This test, to make sure that an exception is thrown when a non-existent migration class is trying to be registered, got moved to MigrationRepositoryTest. Because the finders use reflection now, this stuff doesn't work. This test worked because the namespace for Version123 was different. Now migrations just loads that stuff for you.

chrisguitarguy added some commits May 20, 2018

Remove an Unused Variable
Leftovers from before refactoring.
@jwage

This comment has been minimized.

Copy link
Member

jwage commented May 21, 2018

@chrisguitarguy Couldn't this change be made BC? Basically, keep on allowing a namespace to be passed optionally and if it is passed, filter the migrations it finds, to only the ones in that namespace.

@jwage jwage added this to the 2.0 milestone May 21, 2018

@jwage jwage added the Enhancement label May 21, 2018

@jwage jwage assigned jwage and chrisguitarguy and unassigned jwage May 21, 2018

@chrisguitarguy

This comment has been minimized.

Copy link
Contributor

chrisguitarguy commented May 21, 2018

Couldn't this change be made BC? Basically, keep on allowing a namespace to be passed optionally and if it is passed, filter the migrations it finds, to only the ones in that namespace.

Probably? It's not really a BC break as it is now: whatever migrations a person has that already exist before this change will still load correctly.

Maybe a better way to make it more BC friendly is to filter out anything that doesn't extend the AbstractMigration so nothing unexpected gets loaded. Maybe also filter out anything that's an abstract class.

@jwage

This comment has been minimized.

Copy link
Member

jwage commented May 21, 2018

@chrisguitarguy The reason I ask is because, I think as it is right now, the migration namespace setting goes unused. So we either need to deprecate and remove that setting or keep it there. I think we should keep it there and enforce the filtering with that setting and make sure it is optional everywhere. This way things are guaranteed to just keep working the way they did before.

@chrisguitarguy

This comment has been minimized.

Copy link
Contributor

chrisguitarguy commented May 21, 2018

I think as it is right now, the migration namespace setting goes unused

It's used in code generation. So another path could be renaming the setting to default_namespace or something like that.

As long as existing migrations load correctly (which they already won't do to void typehints added to AbstractMigration::{up,down}), then I wouldn't add another flag option to enable or disable this stuff.

@chrisguitarguy

This comment has been minimized.

Copy link
Contributor

chrisguitarguy commented May 21, 2018

Unless you want to target something in a 1.7.x release that deprecates things -- similar to how you have to opt into behavior into new behavior symfony upgrades (thinking specifically of some form stuff).

@jwage

This comment has been minimized.

Copy link
Member

jwage commented May 21, 2018

I would prefer to keep it all BC and not remove or rename anything.

protected function getFileSortCallback() : callable
/**
* Look up all declared classes and find those classes contained
* in the give `$files` array.

This comment has been minimized.

@jwage

jwage May 22, 2018

Member

Typo give should be given

};
$classes = [];
foreach (get_declared_classes() as $class) {
$ref = new ReflectionClass($class);

This comment has been minimized.

@jwage

jwage May 22, 2018

Member

We like to avoid using abbreviations. Please use $reflectionClass as the variable name here.

@@ -12,12 +12,12 @@
/**
* @return string[]
*/
public function findMigrations(string $directory, ?string $namespace = null) : array
public function findMigrations(string $directory) : array

This comment has been minimized.

@jwage

jwage May 22, 2018

Member

As discussed, please reintroduce the $namespace argument as optional and if it is provided, filter the classes returned to only be the ones in that namespace.

@chrisguitarguy

This comment has been minimized.

Copy link
Contributor

chrisguitarguy commented May 26, 2018

@jwage changes made! Sorry it took me so long to get back to this.

Do we need to add the configuration option here to enable multiple namespaces?

@jwage

This comment has been minimized.

Copy link
Member

jwage commented May 29, 2018

@chrisguitarguy We could do that in another PR to allow multiple namespaces to be specified for the filtering. I think this is probably fine for this PR.

@jwage

jwage approved these changes May 29, 2018

@jwage jwage merged commit 956cdf5 into doctrine:master May 29, 2018

2 checks passed

Scrutinizer Analysis: 1 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment