cviebrock / eloquent-taggable

Easily add the ability to tag your Eloquent models in Laravel.
MIT License
559 stars 72 forks source link

Fixes #99 #104

Closed mateusjunges closed 4 years ago

mateusjunges commented 5 years ago

This is related to #99:

@judgej It would be nice if you can review this PR.

mateusjunges commented 5 years ago

Thanks for your review @cviebrock! I'll update the PR with your suggestions.

mateusjunges commented 5 years ago

@cviebrock do you want any changes in this PR?

cviebrock commented 5 years ago

I'm getting itchy feet about the fact that the schema is changing for the tables, and how this is going to affect users who just upgrade the package but still have the old schema (specifically tag_id being renamed to id).

I'll also admit (like @judgej mentioned) to not seeing an custom migration configuration like this in any other packages, even ones where you can publish the config. I'm looking at https://github.com/spatie/laravel-activitylog/blob/master/src/ActivitylogServiceProvider.php for an example package that has a configurable table and how it handles it in the migration and service provider.

mateusjunges commented 5 years ago

@cviebrock I removed the custom_migrations flag. In general, packages that allow users to publish migrations don't load them before they are plublished. I think it is not a good approach, since if i don't want to customize my migrations, also i don't want to publish these migrations. To solve this, i check if the migrations have been published and load conditionally: if published, load from database_path(), and, if not, load from this package directory.

 $package_migration = glob($this->app->databasePath()."/migrations/*_create_taggable_table.php");

if (count($package_migration) > 0) {
    $this->loadMigrationsFrom(database_path('migrations'));
} else {
    $this->loadMigrationsFrom(
        __DIR__.'/../resources/database/migrations'
    );
}
mateusjunges commented 5 years ago

@cviebrock Any thoughts on this?

cviebrock commented 4 years ago

I am finally getting around to this.

I won't merge this PR for a few reasons, mostly because of the backwards incompatibility with the schema.

However, I am pushing a new version (6.0.2) that includes most of the work you did in terms of updating the keys, making the table names configurable, and making the migrations publishable ... so thank you for all that work! Let me know if there is anything I've missed.