Add env var for setting livewire tmp file upload disk

[craigpotter]

1️⃣ Is this something that is wanted/needed? Did you create a discussion about it first? I didn't but it's a simple change

2️⃣ Did you create a branch for your fix/feature? (Main branch PR's will be closed) Yes, the branch is named craigpotter:add-tmp-env-var

3️⃣ Does it contain multiple, unrelated changes? Please separate the PRs out. No, the changes are only related to my feature.

4️⃣ Does it include tests? (Required) No, let me know if you want a test but I don't think it is required.

5️⃣ Please include a thorough description (including small code snippets if possible) of the improvement and reasons why it's useful.

This is a pretty simple change, it just adds a env var to change the temporary file upload disk.

In our case, we wanted to use a dedicated disk as we were using S3 and didn't want to use the default disk. We had to publish the entire config to just change that one setting.

Now we have to maintain the published config when upgrading livewire. Having a simple env var to set the disk allows developers who need to change the disk to not have to publish the entire config.

env() returns null by default so this is not a breaking change for anyone who doesn't have that env key

[lrljoe]

You should specify a default of null as the second parameter in env(), to avoid any sast tool throwing a hissy fit, or stricter config throwing an error.

But in my opinion, publishing the config is a much more sensible approach if you want to customise. You don't need to have the whole config present, as it is merged with the default config, only the values you've changed from the defaults.

[schonhoff]

You should specify a default of null as the second parameter in env(), to avoid any sast tool throwing a hissy fit, or stricter config throwing an error.

But in my opinion, publishing the config is a much more sensible approach if you want to customise. You don't need to have the whole config present, as it is merged with the default config, only the values you've changed from the defaults.

Hey, the env()-helper has the second parameter null per default. Here is the function header:

// helper
    function env($key, $default = null)
    {
        return Env::get($key, $default);
    }

// Env::get
    public static function get($key, $default = null)
    {
        return self::getOption($key)->getOrCall(fn () => value($default));
    }

There should be no need to add null as a second parameter. If I would do it my IDE is yelling at me for using a default value. Laravel is more and more using env-variables for configurations instead publishing config files. Maybe it is a good approach to slowly changing the config of livewire to .env variables were it is possible.

[lrljoe]

Using env within config files is sensible and recommended, as these are cached.

Using env outside of config files is not recommended really, it adds a barrier to implementation as it's not centralised, and makes it harder to maintain longer term.

Just reviewed the env helper for the livewire supported laravel versions, and yep, you're right that it defaults to null in presently supported versions.

To give another use case, a multi-tenant app may override config settings on a per-tenant basis. Using env values directly would introduce more complexities, and potentially expose some env based vulns.

100% support making things more configurable, but config files are definitely the way for now.

@schonhoff Where are you seeing evidence of Laravel using raw env variables rather than config files?

[schonhoff]

Hey @lrljoe!

This PR adds the env helper to the base config file for livewire. As you mentioned this is recommended and it is done by Laravel with Laravel 11.x release a lot.

Your multi-tanent app would still work because you can still publish the config file and change the array value from the env helper to whatever you want.

Maybe I'm missing something here.

[craigpotter]

Also just to add

You don't need to have the whole config present, as it is merged with the default config, only the values you've changed from the defaults.

This is fine apart from you can't just maintain one key in the array. The config is merged in Laravel's mergeConfigFrom which uses the array_merge function.

If someone published the config and deleted everything apart from temporary_file_upload.disk it would remove all the config for temporary_file_upload.rules and the other subkeys

Here is a very basic example.

Therefore you need to maintain the published config making sure to add any sub keys which might have been added.

Having an env var for this, means you can override it without having to publish the config.

In your multi tenant app use case, could you explain how it cause more complexities or security vulns? Surely it is no different to how it is now. If you publish the config, it will have the default value or a value you have set. In Multi Tenant, you would just use the config helper or something to overwrite that value once you have identified the tenant?

I don't see what is different other than it gives developers a way to change that default value without having to publish the entire config. And if you don't like it and think it introduces some security vuln... you just publish the config and remove it.

I feel like it will help more people to have env vars in the config than not.