Laravel-activitylog: Use default config instead of env as connection

[Gummibeer]

This issue relates to https://github.com/stancl/tenancy/issues/164

The idea is to drop the double env() call in package config and use package env or null as default value. In model construct we use the database.default as fallback value.

PS: I would prefer null coalescing operator over function nesting.

// good
config('key') ?? config('key') ?? 'default';
config('key') ?? config('key', 'default');

// bad
config('key', config('key', 'default'));

[micc83]

Even though tests are green, I see that config('activitylog.database_connection') is clearly used also in the migration. Unless I add the same logic there as well I would break the package. But, in relation to https://github.com/stancl/tenancy/issues/164 and as I'm not very familiar with this scenario, I'm not sure how you would handle migrations for multiple tenants. Is that migration re-run for each new tenant with an updated database.default config?

[Gummibeer]

@micc83 Good catch and thanks for doing it! :)

@stancl could you help how the migration should be handled?

https://tenancy.samuelstancl.me/docs/v2/tenant-migrations/
It seems like there is a dedicated command for it and the migration has to be placed in a new directory. So the user has to adjust it anyway. In this case I would say that we should only pass the package connection and null as default. The connection() method accepts a string or null and uses the default connection if connection name is null.
So this should be the same like we do in the model construct!?

And something I've noted after taking a look in the code - we could omit the fallback at all. If our package config is null it will use the app default connection. So we only pass in the configured explicit connection or null and let the resolver do its job. This will make it even more consistent with the migration and we don't tinker around with default behavior.

[micc83]

@Gummibeer I'll reply above point by point:

1. So this should be the same like we do in the model construct!?

So the user will have to manually copy the CreateActivityLogTable migration to the tenants folder anyway, right? Let's wait for @stancl for more info about it.

1. We could omit the fallback at all

I see that both DB::connection($param) and Model::getConnection($param) already fallback to database.default when $param is null so, unless there's some other scenario I'm not considering, I'm reverting my change on the App\Activity model to use only config('activitylog.database_connection').

I'll update my fork in a minute.

[Gummibeer]

The last point was what I meant. So yes, go for it. And the second seems right to me after reading the docs for 5min.

[micc83]

@Gummibeer great, check the commit on my fork. If it's ok, while waiting for @stancl feedback I'll open a PR.

[stancl]

we could omit the fallback at all

This is what a lot of Laravel components/packages do. The published config has no connection key, so it's implicitly null, but it can be set to a specific value.

So this should be the same like we do in the model construct!?

Yes. All my package needs is a fallback to 'default'.

With regards to migrations, they use the database.default connection. They need to be in a special folder just to distinguish them as tenant migrations.

@drbyte Pointed out that he'd want to use the activity log both in the central and the tenant parts of his application. The fallback to default makes that possible.

This use case also requires that the migration is included in both folders.

[micc83]

@stancl fair enough, so we should be good to go. I'm opening the PR.

[Gummibeer]

https://github.com/spatie/laravel-activitylog/releases/tag/3.9.1