Add a Production/Development Toggle To Core

Just to note that we do things like this: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...

    // Indicate that code is operating in a test child site.
    if (!defined('DRUPAL_TEST_IN_CHILD_SITE')) {
      if ($test_prefix = drupal_valid_test_ua()) {
        $test_db = new TestDatabase($test_prefix);
        // Only code that interfaces directly with tests should rely on this
        // constant; e.g., the error/exception handler conditionally adds further
        // error information into HTTP response headers that are consumed by
        // the internal browser.
        define('DRUPAL_TEST_IN_CHILD_SITE', TRUE);

        // Log fatal errors to the test site directory.
        ini_set('log_errors', 1);
        ini_set('error_log', $app_root . '/' . $test_db->getTestSitePath() . '/error.log');

        // Ensure that a rewritten settings.php is used if opcache is on.
        ini_set('opcache.validate_timestamps', 'on');
        ini_set('opcache.revalidate_freq', 0);
      }
      else {
        // Ensure that no other code defines this.
        define('DRUPAL_TEST_IN_CHILD_SITE', FALSE);
      }
    }

...when we could do things like:

$kernel = new DrupalKernel('kernel_test', etc);

Of course it's much later in the game than comment #18, approx 12 years ago, which was the time to adopt this model. Risk-averse maintainers probably don't want to untangle it now.

Speaking of which, I think DRUPAL_TEST_IN_CHILD_SITE might have survived all the way from Drupal 7, which means evolutionarily speaking it's the fittest.

xjm → credited bnjmnm → .

xjm → credited catch → .

xjm → credited lauriii → .

xjm → credited longwave → .

@catch, @longwave, @alexpott, @lauriii, @bnjmnm, and I discussed this issue at DrupalCon Barcelona 2024.

Previously on this issue, we discussed the possibility of allowing two values, three values, or an arbitrary number of user-defined values for different site environments.

Today we discussed and agreed that we will restrict this to two modes (dev and prod, or along those lines), for the following reasons:

• "Staging" or "QA" means different things to different sites and organizations. If we provide this middle option, developers and site owners will make assumptions about what it means in terms of which production behaviors you get. (E.g., that a site in staging won't send email, or that it will but that the mail client will handle the test emails, etc.). These misunderstandings could have serious consequences for site data and operations.
   

• It's comparatively easy for a site owner or developer to understand what a site being "dev" means and how that differs from production.

We also discussed where this flag should be stored:

• It should not be in config because it is not something that you deploy.
• It could be stored in either settings.php or services.yml
• We could optionally allow overriding it in state or similar (so that there would be both filesystem and database methods for overriding it).

#75 in particular is out of scope for the dev/prod toggle we're discussing here.

Maybe it's bikeshedding, but terms I usually here in this context are a bit more general: rather than "dev"/"prod" the distinction tends to be "build" vs "run", which may be more explicit: it's either an environment in which devs intervene, or one on which they (usually) don't.

Also, as to where, the most logical location for something which is deployment-dependent seems to be settings. The bigger plus is that it can be updated without downtime by a single file change. The minus being that it conditions modules and services availability. "Run" deployments usually have dedicated cache servers, queue servers, proxy, CDN, which may not be enabled on a "Build" environment, and that means differences from the composer.json up, with different resulting modules and configurations, which is not something that something in settings can support.

The way I've been doing it on client projects since Drupal 8 pre-alpha has always been to expose this in the composer extra information, so it's available from the build step, AND use it to generate two per-environment settings using (Twig) templating, so all scripts and composer plugins run relative to that environment. Deployment commands, being "build", use the "build.settings.local.php" target, while everything after build (hence run) uses the "run.settings.local.php" target, switching being performed because the "settings.local.php" is just a symlink that can be updated atomically.

Given we decided this would only be a boolean, I think we could make it $settings['development_mode'] = TRUE; (or however mechanism we define it) and then it would really be a development mode toggle, then leave exactly what it means when it's FALSE implicit.

Converted the latest patch to an MR.

One objection to a setting: IIRC, settings jump in quite late. A container parameter would be better in this regard.

And a much bigger objection: The MR contains @dawehner's quich shot from #58 from 7 years ago, and is a strong Drupalism.

Let's instead leverage the kernel.environment container parameter. It is symfony standard and already heavily supported in our Drupal kernel:
- \Drupal\Core\DrupalKernel::getKernelParameters
- \Drupal\Core\DrupalKernel::getContainerCacheKey

To be honest I'm fine with keeping this a setting. I don't regard Settings::get() as that bad of a Drupalism, or at the very least don't think this issue should be held up by it.

If we want to move away from using Settings in favor of container parameters, then we need to first agree on that in a separate issue and come up with a plan to remove all use of Settings. But then you'd have to inject the container into every single service that might need to check for a setting. There's also a difference between what you can do in a settings.php file and a services.yml file.

As you can see, many things left to discuss on that front. So maybe let's be fine with adding "yet another" setting here and save that discussion for another issue?

Will rerun the pipeline as there seem to be random failures. Keeping at NW for a hook_requirements test, but that should be straightforward.

Can't seem to rerun the pipeline for this MR (got push access).

I couldn't run the pipeline either. Then I saw that some of the linting had failed, so I reran those and then the tests ran. There were lots of failures but there are no test reports. Odd.

On the good side the pipeline and reporting is working as expected now.

Could we add test coverage for this?

Only concern is I know the new development settings was meant to not be captured in config/repo. This change can kinda undo that

@smustgrave, can you elaborate on what a test would be testing?

Probably checking that the setting does what described. And message appears on the status report page

We don't test that many settings for their values, do we? I know we have some tests for showing up on the status report, but not for the actual settings value.

This would be set in settings.local.php (or equivalent) so it won't end up in config or version control.

We will need to add test coverage when we actually use this in places, but that's not introduced in this issue. Tests for the status report is fine though.

As discussed already in the issue, 100% agree to this being only a bool, then it's up to developers where they actually set it on or off according to workflow.

Two questions:

1. Should this definitely be a setting in settings.php and not a container parameter?

2. I'm not sure we should say this is unrelated to settings like javascript aggregation and twig debug - we may well end up changing the behaviour of those when it's set in follow-up issues. And even if we didn't do that in core, contrib could.

+1 to not being too specific about what this affects. We'll end up having to change that all the time or risk it getting stale.

Tests for the status report is fine though.

There is no mention of the status report in the issue summary. The first mention is in #93.

What is expected to be on the status page?

If I’m looking at the code correctly if the setting is TRUE there will be a warning on the status page

I talked to catch about the needs for tests and we agree that manual testing is fine. If someone wants to write a test, that is OK, but not a requirement.

That leaves the question of whether this should be in settings or a container parameter. There was also the idea of having it in services.yml. See #80.

I did manual testing and added screenshots. They may need to be changed depending on where the value is stored.

Converted to a container parameter.

In slack catch mentioned the UI, so I changed this to container parameter. Is this preferred?

My thoughts with the UI were that people can either directly set it in a local services.yml (where twig debug already is), or that assuming we add this to the new development page, it could be set via that (with key/value in the middle like the other toggles).

The MR does add 'development mode' to the new development page.

For the 1 comment in the MR.

Also seems like will need a CR.

With plans of introducing ✨ Add an API for feature flags Active , and having all those flags already (css aggr, js aggr, twig debug, twig caching....) but this one not affecting those, I'm not sure how this is planned to be used.
Maybe some examples, even if hypothetical, could help.

Not having an instant flag for checking dev vs prod/build vs run is better than having one that might not be consistent between different project/sites. agencies, etc.

This issue is waiting for the current one to land: 📌 Convert trigger_error in VariationCache into a LocigException. Active

The goal is to throw exceptions rather than warnings when on a dev install so that we can catch more VariationCache violations as not all of them are covered by test cases.

Just revisited this issue based on 📌 Require password confirmation to install new code Active and I think we might want to split the UI into its own issue - if we use development mode to suppress password re-authentication, we either need to not have a UI, or put the development mode settings themselves behind password re-authentication.