Add setting that can be overridden to disable API purge for local environments

Re-rolled #16 for 4.x

vvuksan-fastly → closed merge request !17

Used this patch for a while now. It seems like the configuration added in the test does not properly deactivate the diagnostics for the purger plugin. It may has to do with the runtime measurement injection, see comment for the hasRuntimeMeasurement() method.

When invalidating a lot of tags on a development environment with the purge option disabled, one may run into errors from the QueueSizeDiagnosticCheck. For example:

Your queue exceeded 100 000 items! This volume is extremely high ...

I do not know the plugin system for the purgers well, yet. Maybe we could make the return value of the hasRuntimeMeasurement() in the FastlyPurger plugin depend on the configuration.

Hey folks. In the light of that config_split provides support for splitting the configs and managing all aspects of the configuration I will close this issue.

https://www.drupal.org/project/config_split →

@vvuksan-fastly Could I possibly ask you to reconsider? Most modules have an off switch of some kind that can be controlled via a settings.php file, and config_split isn't that commonly installed. I just checked some statistics, and the most common contrib modules have 300k-600k reported installs, while config_split only has 60k. Our site doesn't use config_split, so we'll now be in the position of installing it just to support this one module (which means a fairly significant adjustment to how all our developers work) or maintaining and running this patch forever. Honestly we'll probably choose the patch route because it's less disruptive.

Raising hand for reconsideration. We'll also be running this patch forever. As stated elsewhere, config_split is not a valid approach because it provides no mechanism to leave the module enabled on local but having the features disabled just through configuration. Puzzlingly enough, for the config_split approach to work, it would actually also need the patch from this issue.

I would also like to see this reopened. Config Split cannot address the issue that uninstalling Fastly & related Purge modules, in an environment lacks the relevant credentials secrets, produces a ton of noisy errors related to inability to purge.

+1 to have this feature without installing config_split

Here's another vote for reconsidering this decision. I agree with the points made above. Config_split is a very non-optimal way to solve this problem.

Re-rolled #16 for 4.3

Also +1 for adding this. Much easier to have code like this rather than having to set up a config split.

// Disable Fastly purging by default.
$config['fastly.settings']['purge'] = FALSE;
if (is_production()) {
    // Enable purging on production.
    $config['fastly.settings']['purge'] = TRUE;
}

Hi @vvuksan-fastly,

I also don't think the decision was correct because 1) config_split is not always the way to go, and 2) best practice for modules similar to fastly is the approach suggested above. Could you please consider it again?

Thank you.

@vvuksan-fastly please forgive me for re-opening this, but I understand closed issues won't get much attention.

Fastly is definitely a niche module and doesn't apply to all projects, but I do agree with all of the above people. Maintaining a split has always proven to be cumbersome and it is something we prefer to avoid for simple things.

AdvAgg, another performance related module supports disabling the module on settings.php level (source: https://git.drupalcode.org/project/advagg/-/blob/5.0.x/src/Form/Settings....

In reality, Fastly is something you will only need on a live environment. It would be great to be able to do just that on module level, and not require an overly complicated extra module.

Again, forgive me for opening this again, feel free to close it again and I believe we'll all understand and stop requesting this. I certainly hope you reconsider though.

Thanks

Like many others here, I don't think forcing users to turn off purging on cache rebuilds only by means of disabling the entire module (and thus all its services) is the right approach at all. Almost all other modules that connect to an external service have a means of turning production functionality off if either a setting is set, or an environment variable is unconfigured. One shouldn't have to install config split and then set up different configurations just to ensure that purges aren't triggered in testing and dev when they only lead to errors.

Nor should we be forcing developers to have to set up a Fastly service just for QA or just for Dev. That's a lot of extra cost and very very few developers get to make purchasing decisions anyway.

Here's another use case. I have written a drush command that depends on the presence of some of the services defined in the Fastly module. I need my custom drush command available in dev for testing. Which means, that the Fastly module needs to be enabled in dev for my testing to work. If I disable the Fastly module, the code I have that depends on the services defined in the Fastly module fails. In fact, I can't even deploy the site due to missing service dependencies if the Fastly module is disabled.

Having a Fastly purge ON/OFF switch makes sense, makes everybody's life easier, and is necessary in cases where other code depends on the Fastly module. It is not hard to implement. Patches have been contributed. The resistance to this feature is really strange.

Said in another way, the concept of feature flags in Drupal are implemented as $config['module.settings']['enabled'] = FALSE;. Those feature flags can then also optionally be exposed in the admin UI, to facilitate quick debugging. Ultimately though, adding a module settings configuration is the only realistic approach to solving this.

The patch in #29 does exactly this. If I had to criticize it at all, it would be to rename the setting 'purge' to 'enabled', to match the property name introduced in the FastlyPurger class.

Another voice to say that config split is a very poor solution for this problem, in spite of its general usefulness.

But config_split or patch, I think this issue is the result of a related issue in the module 🐛 Api::validatePurgeCredentials() can fail in non-production environments Active having to do with its use of Drupal's state system, and how a certain state value can remain unexpectedly true in non-Prod environments.