Internal migration

[Manriel]

Package have internal migration included.

It is hard registered inside the Serivce Provider and have own timestamp: 2018_08_11_003343_create_log_table
This leads to behavior that breaks normal flow of artisan migrate command in case the log table migration already exists with modified structure or should be created by migration leaded after that.
For example of modification: if the table needs to be separated to some kind of partitions, but it required to be exists before some previous migrations by the current migration's timestamp.

Current behavior force to do the manual per-file migration to avoid this problem.

I see several ways to fix this behavior:

1. Simplest solution: wrap migration by if (Schema::hasTable(...) { ... }. That is easiest but not the better way, 'cause it hides the migration from a dev and imposes some restrictions.

2. Better solution:

• 2.1. Remove the registration of the migration from the Service Provider

• 2.2. Add another one directive like directive with config to publishes a copy of the migration to common migrations directory on the normal laravel structire to allow modify it by dev's needs.

• 2.3. Rename migration to give it inert timestamp 0000_00_00_000000_create_log_table like it doing some of built-in migration of the framework. This kind of name of migrations should make it started 1st and when you publish it, it will appear at the beginning of the migrations list, that allow devs to catch sight of the migration and rename it giving timestamp they need or leave it as is.

[Manriel]

I chose not to create a pull request as far as better solution is the changing of the migration name and it place, that is a breaking change, that force devs who use this lib to update their code, do the publish of the migration and change it name if it will be need.

It should be better to discuss about the changes here before to implement it.

[danielme85]

Hi @Manriel, let me think about this... wouln't you just make a new migration to edit the log table as needed? said migration table would have a later timestamp than 2018? I diddn't really consider the datetime in the migration.
https://laravel.com/docs/8.x/migrations#updating-tables

If you manually modify table structures, that's always gonna make the migrations not so happy at some point... I mean I've had my fair share of migration issues because I took (and still do) shortcuts to modify the table, or edit migrations that has allready been run. However that is not the way... making a new migration for every change: This is the way! Tidious, yes...
https://www.youtube.com/watch?v=uelA7KRLINA

It's currently added as per Laravel documentation: https://laravel.com/docs/8.x/packages#migrations
Adding a check too see if the table allready seems like a good idea, I don't see the harm in that. I checked to see but I don't see any mention of migrations when it comes to vendor:publish in the docs.
I guess migrations could also be included in a publish command the same way config/views etc are...
just slap the migration into here
$this->publishes([ __DIR__.'/../resources/views' => resource_path('views/vendor/courier'), ]);

However I don't know what the "norm" is here, I might too some research/looking at what other devs do.
I'm ok with it beeing one more install step, however like you mentioned: it might be a breaking unexpected change if people use this in deployments etc.
Oh wait... I just found this in the documentation:
https://laravel.com/docs/8.x/packages#publishing-file-groups
Would you look at that, can be published as a migration tag. I guess I never got that far in the docs :p

Still the question is: making the migration optinal: is that a big breaking change? The code won't need to be changed: but there would be an additional php artisan vendor:publish laravel-log-to-db:migrations or something like that command when you install the pacakge.
Maybe I just bump to a major version since there is potential for breaking deployments...

PS, Also: I forgot about this, there was a PR to add config('logtodb.collection') to the migration, so you can rename the table to something else if you want to avoid conflicts in the meantime.

[Manriel]

wouln't you just make a new migration to edit the log table as needed? said migration table would have a later timestamp than 2018? I diddn't really consider the datetime in the migration.

I have a project, where the timestamp of migrations started from 2014. And it happened so, that in some stage of the app life cycle the self-made logging lib has been replaced by this logging lib. So far as it's absurdity to create table in one migration then delete it and re-create in later migrations, old migration became not need anymore for app deployment, and it has been replaced by the migration of this lib for deployment of the app in future.

However, at some point we were faced with the fact that the migration should start one of the first, since subsequent migrations are logged that requires the log table exist, but the current migration timestamp turned out to be later than it required. When we changed timestamp, we realised, that the migration became starting twice: our own with change about table partitioning and that one, that hardcoded to the lib. We can't control, change or avoid migration that is hardcoded to the lib.

Adding a check too see if the table allready seems like a good idea, I don't see the harm in that.

Absolutely agree with that. But it wouldn't a complete solution.

Would you look at that, can be published as a migration tag. I guess I never got that far in the docs

That is exactly what I meant when wrote about adding directive to publish a copy of migration. Surely, it needs to be tagged.
Publishing of the migration is the better way, cause it make lib more flexible and keep control over migrations to a dev. This is especially important for a table with logs, 'cause in some cases the number of logs can exceed all reasonable limits, but they must be saved still, and dev need keep strongly control over the storage in that case.

Also, I absolutely sure that some migrations was publishing with inert timestamp like 0000_00_00_000000 previously, cause some of them exists in few projects under my development. Unfortinatly, I didn't found examples in fresh laravel repos right now.
Anyhow, I found brilliant implementation for migrations and timestamps in one of laravel modules:
https://github.com/laravel/cashier-mollie/blob/fbf6fff2fc55abf43cc00df5c7542d41a09fcf2b/src/CashierServiceProvider.php#L76

Still the question is: making the migration optinal: is that a big breaking change? The code won't need to be changed: but there would be an additional php artisan vendor:publish laravel-log-to-db:migrations or something like that command when you install the pacakge.
Maybe I just bump to a major version since there is potential for breaking deployments...

Bumping a major version is the way. :-)
It wont affect on new installations except adding one more step, but it will definitely break projects that already use current version of the lib and dev will want to update it. So, this change must be presented in the upgrade notice.

[danielme85]

Aha, I see what you mean now.
Thank you for bringing this to my attention and giving me all the details and suggestions to make improvements!

I'll see if I can't add these fixes this weekend, and make a major version update and add additional information to the readme.

[danielme85]

I'll probably pull this to master and create a new release version tomorrow after a practical test.
#33

[Manriel]

Thank you @danielme85.