pingpong-labs / modules

Laravel 5 Modules
https://pingpong-labs.github.io/docs/modules.html
BSD 3-Clause "New" or "Revised" License
576 stars 151 forks source link

Migration class not found #168

Open nWidart opened 9 years ago

nWidart commented 9 years ago

Hello,

On thing I have since the update of earlier (>2.0.8), is a migration error:

Class 'CreateSettingTranslationsTable' not found  

But never had this before this update ... Also this migration has already run, and is already in the migrations table.

This all worked fine before on 2.0.8.

nWidart commented 9 years ago

I've tried merging the migration from settings translation to another migration, and then I get:

Class 'CreateSettingsTable' not found

There is definitely something wrong with the package.

nWidart commented 9 years ago

I just tried using "pingpong/modules": "2.0.8",, and here all works fine.

nWidart commented 9 years ago

It's very likely something in this commit: https://github.com/pingpong-labs/modules/commit/84b78dfd8bc6d85bb6927c30af324a15aebede9b

Even after running module:publish-migration, which by the way publishing in the app folder instead of database/migrations, it still says the same error.

nWidart commented 9 years ago

First of all, you expect a module name here: https://github.com/pingpong-labs/modules/blob/84b78dfd8bc6d85bb6927c30af324a15aebede9b/Commands/MigrateCommand.php#L56 But you send it the full module class. So that can't work.

Second, you have:

$migrations = array_reverse($this->getMigrations());

But that can't work, since in this example, create_settings_translations needs to be run after create_settings, because of foreign keys.

If you really want to remake a migrations command (why?), you should checkout : https://github.com/illuminate/database/blob/master/Migrations/Migrator.php#L236

nWidart commented 9 years ago

From what I see, you do not require the migrations before new $class. You have a public function requireFiles($path, array $files) method that's never called.

Also, you don't check for migrations that are already run.

This:

$ran = $this->repository->getRan();
$migrations = array_diff($files, $ran);
nWidart commented 9 years ago

With this: http://laravel.io/bin/rooP9 it works, I think. Please test stuff extensively before tagging.

Also why did you copy (parts) of the Migrator class of laravel ?

Why don't you use, the following and loop over all modules to get the migrations path:

php artisan migrate --path=/Modules/Setting/Database/Migrations
gravitano commented 9 years ago

I am not copying the Migrator class from Laravel. I just re-create it for solving this issue #153.

nWidart commented 9 years ago

Could you please make it work than?

Thanks,

On Saturday, June 6, 2015, Gravitano notifications@github.com wrote:

I am not copying the Migrator class from Laravel. I just re-create it for solving this issue #153 https://github.com/pingpong-labs/modules/issues/153.

— Reply to this email directly or view it on GitHub https://github.com/pingpong-labs/modules/issues/168#issuecomment-109503110 .

Nicolas Widart www.nicolaswidart.com

nWidart commented 9 years ago

I see you did not remove array_reverse. This cannot work! If look at the Migrator class from Laravel, array_reverse is only used in rollback and reset.

I see you tagged this, again without testing...

nWidart commented 9 years ago

If you look at this: https://github.com/AsgardCms/Setting/tree/develop/Database/Migrations

You see 2 migration files:

What you're doing in the migrate method, is reverse those, so that create_setting_translations_table comes first. But this isn't good since if you look at the migrations create_settings_table needs to go first because of foreign keys being created !!!

Also, please look at the laravel migrator class, which you didn't copy, but is the exact same class, the only place there's an array_revers is in reset and rollback!!!

nWidart commented 9 years ago

FYI: working version: https://github.com/nWidart/modules/blob/hotfix/migrations/Migrations/Migrator.php