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

Fixed a number of bugs in the module:migrate + module:seed commands #46

Merged
merged 6 commits into from
Sep 14, 2016

Conversation

Schweppesale
Copy link
Contributor

@Schweppesale Schweppesale commented Sep 14, 2016

Refactored the "module:seed" command logic in order to reduce code duplication.

Fixed an existing bug which would cause the "module:migrate" command to throw an offset warning if the --seed option is specified.

Fixed a newly introduced bug(my bad) where the SeedCommand:dbseed() method would append the base module namespace to the FQN of a seeder class. This would only occur if the user had provided an explicit classname using the optional "migration.seeds" argument.

Removed the "class option" from "module:seed" since it should logically only be set from within the dbSeed($class) method which in turn issues each of these calls directly to the "db:seed" command.

Removed the "all option" from "module:seed" since it actually wasn't being used for anything.
ie: The existing behavior for this command is to seed all modules if the module argument is not specified.

Removed the MigrateCommand:getPath() method in favor of (Migrator($module))->getPath() which has been updated to account for the additional logic.

Note: Since the Laravel "migrate" command requires a path which is relative to the base_root() module:migrate will still executing a string replacement on the result.


Update:

In order to further reduce code duplication I made some more changes to the MigrationPublisher class which will now accept Migrator as a class dependency.

Calls to the MigrationPublisher:getSourcePath() method should be delegated to the Migrator:getPath().

Note: It doesn't look like this class even needs to extend the AssetPublisher. It could probably just extend Publisher but I decided to leave it as is for now.

@nWidart
Copy link
Owner

nWidart commented Sep 14, 2016

Thanks again!

I'll have to try these out.

@nWidart nWidart merged commit 0dd9100 into nWidart:master Sep 14, 2016
@nWidart
Copy link
Owner

nWidart commented Sep 14, 2016

Looks like on my simple tests.

Could be nice if you can PR the readme file to add info on how to overwrite the migration/seed path. :)

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.

None yet

2 participants