Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[11.x] Add option to disable merging of base configuration #51579

Merged

Conversation

taka-oyama
Copy link
Contributor

@taka-oyama taka-oyama commented May 27, 2024

Addresses: #50876

Context

#47309 added defaults to various configurations which caused our tests to fail.
For example, we have an inspect command which scans through and connects to external APIs such as database.connections and queue.connections to make sure that all connections are healthy.
This change broke that and other similar commands that scan the configuration and connect to them.

I've added Illuminate\Foundation\Application::dontMergeBaseConfiguration() which can be called during the bootstrap phase that will allow me to disable the merging of base configuration.

@mfn
Copy link
Contributor

mfn commented May 27, 2024

I very much support this PR or any solution which allows me to disable this merging completely.

I strongly believe this "feature" should never have made it into the code base, it's diametral to "least surprise" for devs.

Thank you!

@mfn
Copy link
Contributor

mfn commented May 27, 2024

@taka-oyama

which can be called during the bootstrap phase

Do you mind sharing an explicit example how to do that?

@taka-oyama
Copy link
Contributor Author

taka-oyama commented May 27, 2024

It would probably look something like below.

<?php
// bootstrap/app.php

use Illuminate\Foundation\Application;
use Illuminate\Foundation\Configuration\Exceptions;
use Illuminate\Foundation\Configuration\Middleware;

$app = Application::configure(basePath: dirname(__DIR__))
    // ...
    ->withExceptions(function (Exceptions $exceptions) {
        //
    })->create();

$app->dontMergeBaseConfiguration()

return $app;

I guess I can clean this up a bit by adding the same method to ApplicationBuilder, but I use the bootstrap/app.php format from 10.x so I personally didn't need it.

@taka-oyama
Copy link
Contributor Author

Thanks for fixing the comments / formatting!

@taylorotwell taylorotwell merged commit 85dbab2 into laravel:11.x May 30, 2024
28 checks passed
@jasonmccreary
Copy link
Contributor

I try not to leave comments which may bring down the vibe of a PR. Generally speaking, I believe the framework should be flexible. Especially in its runtime configuration.

With that said, I would advise against using this method...

The merging of base configuration is what allows you to slim your Laravel application, as well as get new options for free. As someone who has facilitated over 100,000 Laravel application upgrades, I can say confidently this is a good thing. I have helped countless developers at their wits end wasting days on a random bug that turned out to be a single missed configuration option.

Using this method means you are fully responsible for maintaining all core configuration files in their entirety now and forever.

@taka-oyama taka-oyama deleted the fix/disable-base-configuration-merge branch June 5, 2024 00:49
@taka-oyama
Copy link
Contributor Author

taka-oyama commented Jun 5, 2024

I would advise against using this method...

Do you have an alternate solution?

@donnysim
Copy link
Contributor

donnysim commented Jun 5, 2024

I think there's some missing points here for why some prefer to not merge it. As it stands now, it introduced a bunch of new environment variables, and as someone who upgraded apps back from 4.2, there's been a bunch of times that those variables were renamed. Having a static config and not mergeable removes the necessity to care about those changes on upgrade, which if you don't have that config "cloned" because you use that variable, you might not even know it changed if you don't read or missed it in the upgrade docs, and those included queue, mail, database variables. From me, that's a big no thank you, i prefer to have my variables static. If i need something new, i will put it in my config.

They even split mariadb and mysql configs, do you really thing that's such a great idea to have auto merged?

@mfn
Copy link
Contributor

mfn commented Jun 5, 2024

100,000 Laravel application upgrades

Here's my perspective: my number of installations is… 1

But it's big, over half a decade old and daily actively changing. We need to have total full control of everything and "be in the know" of every change.

That includes, on Laravel updates, we also need to check & diff and the configs (re-trigger writing default configs, review, pick the pieces which can stay, etc.).

It's absolutely critical every piece of information is explicitly known.

Auto-merging some base values not even going to be used but actually introducing problems just doesn't work for us.

@jasonmccreary
Copy link
Contributor

but actually introducing problems

@mfn, can you share more detail on this?

@mfn
Copy link
Contributor

mfn commented Jun 5, 2024

See the original discussion I created, where details, also from others, where shared -> #50876

Only a few selected config parts are affected by this (there's an explicit approved list of configs), but one of those is related to databases and is a problem in certain setups.

From that discussion, this is a good summary and also affects me:

If we've deliberately removed database and cache connections, mail services that don't exist and the like, then having them magically added back can (and in our case did) cause unexpected and undesired outcomes.

Database config for example should never have connections that don't exist.

@jasonmccreary
Copy link
Contributor

@mfn, thanks. I still don't see the problem - such as an error or incorrect overwrite.

What I see is more a desire for full control over the configuration. As I noted in my original comment, that's fine when you assume full responsibility. You and other commenters here may truly understand what that means. I still don't think this method is needed and will likely be a footgun for most.

@donnysim
Copy link
Contributor

donnysim commented Jun 5, 2024

The whole footgun is fake no matter if it's merged or not. if you don't override the config, any changes to config can be breaking, if you override the config with mergeable option, it might not fully merge anyway so you lose those new options. In any case you cannot depend on the new option being present, because even without the merge disable you are not guaranteed to have those new options.

@donnysim
Copy link
Contributor

donnysim commented Jun 5, 2024

For example, take a clean laravel installation, it already comes with configs, now the framework decides to add, say 'lockout' => 60, to the auth.passwords.users config, it will not be present for anyone unless you delete auth.passwords.users or auth.passwords or whole auth config file etc.. The whole concept that framework can magically add new options and it works for everyone is just not true. They might add root values, but even those risk clashing with existing user values and be different in structure etc. and even crash because of it. The whole config merging concept is just nice on paper, nothing more.

@jasonmccreary
Copy link
Contributor

The whole concept that framework can magically add new options and it works for everyone is just not true.

As someone who helped write this part, I have to say not true to your not true. 😉

I do not wish to start a debate. Again, if this is something you all want to do, great. I left my disclaimer for everyone else.

@donnysim
Copy link
Contributor

donnysim commented Jun 5, 2024

I tested it just to be doubly sure, so I'm 100% positive on what i said unless it's misunderstood, but yeah, no point in ranting on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants