Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#50992 closed defect (bug) (fixed)

Consider removing the ability to alter the list of environment types

Reported by: johnbillion's profile johnbillion Owned by: johnbillion's profile johnbillion
Milestone: 5.5.1 Priority: normal
Severity: normal Version: 5.5
Component: Bootstrap/Load Keywords: has-patch has-dev-note
Focuses: Cc:

Description

In #33161 the concept of environment types was added to core. This is a welcome addition and introduces a means of toggling functionality dependent on whether the environment type is development, staging, or production.

The intention of this feature is to provide a consistent means of identifying the environment type, not of identifying a specific environment. There was discussion on #33161 around which types to introduce, including UAT, QA, etc, and ultimately it was decided to stick with development, staging, and production as this covers the types that plugins and themes might be concerned about when toggling functionality. Actual environments should fit within one of those types (eg. a UAT site can be classified as either development or staging, as you see fit).

After spending time investigating ways to implement this feature, it's become apparent that the ability for the environment types to be altered via the WP_ENVIRONMENT_TYPES environment variable or constant is a mistake, as it nullifies the consistency that this feature otherwise introduces. Due to the ability to override the environment types, there's no guarantee that the type of any given environment will be one of development, staging, or production.

I recently discussed this with the folks at WordPress.com VIP and they agreed that introducing new types isn't the intention of this feature: https://github.com/Automattic/vip-go-mu-plugins/pull/1703

I would like to recommend that support for the WP_ENVIRONMENT_TYPES environment variable and constant is removed, and for the list of environment types to remain static.

This would be a breaking change but hopefully not one with great impact.

Change History (31)

#1 @TimothyBlynJacobs
4 years ago

I agree with this 100% for the same reasons. Plugins would have to treat all unknown types as something and what that should be is unclear and could end up different for everyone.

#2 @jeremyfelt
4 years ago

I also agree with this change for 5.5.1.

I used this on a project for the first time yesterday and was a little surprised I needed to redefine the full list to add local (related #51064). I then was worried that if I added my own, I'd have to closely track other uses of different terms.

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 years ago

#4 @batmoo
4 years ago

Howdy! Mo from the WordPress.com VIP team here.

One of the challenges we've run into with the environment type label is that it's quite narrow and conflates different things:

  • Where is this application running? (e.g. local environment or remote environment)
  • What is the application’s purpose (e.g. production site; non-production site; staging site; etc.)

In our case, we have customers running "develop", "preprod", "production" environments (purpose; these can have arbitrary labels) on our "production" servers (location). For most of our customers, all the environments are expected to behave the same way (there should be little to no variation in functionality on the "develop" site, for example). This actually created a problem for us with Jetpack when we integrated this feature as all our non-production environments ended up in Jetpack’s “Staging Mode” which is not what we want. At the same time, there may be plugins (e.g. ones that post to Twitter / Facebook) that would definitely need to be restricted on develop/preprod/staging environments.

Customers may also have "develop" environments (purpose) running local machines or self-hosted servers (location).

Actual environments should fit within one of those types (eg. a UAT site can be classified as either development or staging, as you see fit).

"as you see fit" starts to create ambiguity as well and what we're struggling with around this feature. In many ways, how you interpret "development" vs "staging" vs "production" is going to be subjective which means that plugin authors will implement behaviours that users may not want because their interpretation of "development" or "staging" was different (for example, "staging" could refer to an environment used to verify code changes before they're pushed to production, or an environment where content is prepared and then copied over to production).

If core wants to restrict the number of types, we'll happily support that, but we would recommend setting very clear expectations around what each type represents to remove any ambiguity. For example:

  • "development": a test version of the site running on a local computer or not the target hosting environment.
  • "staging": a test version of the site running on the target hosting environment.
  • "production": a live version of the site running on the target hosting environment.

While these wouldn’t cover all our use cases, we think the clarity and consistency would help.

We may want to consider guidelines for plugin authors as well:

  • Include overrides for behaviour (e.g. a filter or option) whenever taking action based on the environment type
  • Document behavioural differences in plugins based on environment type, ideally in the readme.
Last edited 4 years ago by batmoo (previous) (diff)

#5 @claytoncollie
4 years ago

@johnbillion so this would effectively remove the ability to do the following in wp-config.php?

define( 'WP_ENVIRONMENT_TYPES', array( 'local', 'development', 'staging', 'production' );

and/or

define( 'WP_ENVIRONMENT_TYPE', 'local' );

Last edited 4 years ago by claytoncollie (previous) (diff)

#6 @johnbillion
4 years ago

Clarification: I agree that adding local to the fixed list of environment types would have its uses. See #51064.

#7 @Clorith
4 years ago

I'm torn on this one, the purpose here was to capture the weird edge cases of those who do not want to conform to the default strings many use, and as those relying on checking the environment type should ideally have a fallback if it's an unrecognized type (in a perfect world, of course, it may or may not be what they do).

A quick scan of plugins using the new function already shows that it's mostly used to see if a given string is the environment, to treat it a given way, so allowing someone who wants to call their environment Dolly is still a thing that won't negatively affect anything as I can see it right now.

That said, it does lose its purpose if those strings can't be identified by the code looking it up in the grand scheme of things, so perhaps you're right that a deprecation notice here may be the way to go, however noble the initial intent behind it was :)

#8 follow-up: @TimothyBlynJacobs
4 years ago

those relying on checking the environment type should ideally have a fallback if it's an unrecognized type

What should it fallback to though? Perhaps development? But then if you have an environment that should be more like staging you don't have any way about it.

The environment should be used a proxy for defining certain behavior, which I think only works if the list is constrained.

#9 in reply to: ↑ 8 @markjaquith
4 years ago

Replying to TimothyBlynJacobs:

The environment should be used a proxy for defining certain behavior, which I think only works if the list is constrained.

Agree. If you have multiple environments that are of a similar purpose and have the same capabilities, they should collapse to the most appropriate environment type, and you should handle the differences between them (if they really matter) some other way.

#10 @garrett-eclipse
4 years ago

Random thought, could we constrain the core list of types to ensure extensibility (via plugins, etc) making it consistent. But, on top of this allow the registration of new symbolic types that require being associated to one of the core 3/4 types. This creates a hierarchical structure where the core types ensure developers have a consistent base to associate their features too, while allowing those with more custom needs to be able to extend the core types. If their extended types are popular enough other developers may switch from using core types to these popular custom types if there comes to light more specific needs.

I agree the need for a consistent type and on top of that think we can build a layer of customization without forgoing that promise of consistency.

#11 @johnbillion
4 years ago

What's the use case for that? It sounds like a different use case.

#12 @johnbillion
4 years ago

  • Keywords needs-patch added; dev-feedback removed

There seems to be consensus about removing the ability for the list of environments to be filtered, so let's see if we can come up with a patch.

I think the existing constant can be deprecated, with any unknown environment name being respected in 5.5.1 and then ignored completely in 5.5.2. The feature hasn't been around long but there's no reason to break sites that have already implemented custom environment types.

#13 @GaryJ
4 years ago

In the least granular form, I think the most common would be to be able to distinguish between "treat as live" and "treat as non-live". That may well serve the purpose of staging (purpose) sites being on production hosting (location), but potentially needing to have some feature (like Jetpack) act as though it were on a live site, or not (for auto-publishing tweets, etc.)

I'm not if that fits in with the intent of the original list, but it seems it's naming of "environment" is ambiguous. If the filter is going to be removed / deprecated, then revisiting the values on there would seem to make sense.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


4 years ago

#15 @joostdevalk
4 years ago

I'm tempted to add one more, now that we've added local, that one being offline. After that, I'd say we should indeed go with @johnbillion's suggestion and remove the filtering option.

#16 @joostdevalk
4 years ago

  • Keywords needs-dev-note added

#17 @elrae
4 years ago

The fact that there isn't consensus on what else we should or shouldn't add (local, offline, etc) to me indicates why we need this filter. This isn't something people can easily add/edit/delete, if someone is changing the environment types and adding new ones they're likely a seasoned developer and know exactly what they're doing. Plugins don't have to worry about trying to handling any number of environment types. I don't expect Yoast or any other plugin to know or care what my environment type is, and it shouldn't be doing anything different. By the logic on this ticket why offer _any_ hooks / filters where users could potentially alter and break functionality? Developers should exercise caution any time they write custom code. I'm strongly against removing more flexibility and this change.

#18 @joostdevalk
4 years ago

@elrae the point is that we do want to know, lest we make errors. If we create a table with metadata on your dev site, and you publish that live, you’ll end up with wrong canonical URLs etc.

#19 @elrae
4 years ago

Why would someone create content on a staging site and push the database live without taking into account dev URLs? Typically you should never push content upstream, only downstream and updates made on the live site. Any time we do for some reason (very rare) move a db from stage to live we always take into account that data may not be fully correct. If Yoast is detecting a stage environment and not populating metadata that it does on live, that could potentially cause errors in scripts that wouldn't show up until live. what if my db has settings that don't allow Yoast to create stuff like in 14.0, or if my tables are so large the queries will bog down my db. Now i'm pushing code to my production that functioned differently and things will start dying on production.

#20 @claytoncollie
4 years ago

I agree with @joostdevalk and @johnbillion that we need a defined list of static environments. That is what Core is supposed to do. Set a standard for the entire community. If the list is able to be changed, then this entire set of functionality might as well not exist. One plugin will call it stg and another will call it staging or 'stage' or whatever else they want. If they are allowed to be changed, the only standardization that will happen is at the host level, where if your site is hosted on say WP Engine, they set the environment definitions for you. But what happens when you transfer you site to another host and they use a different set of terms? It is completely disjointed. This feature/list of environments is solving a real problem that the largest plugins and hosting providers are running into. Yoast, JetPack, WooCommerce. I am more than happy to follow their lead on this since they have built such amazing, widespread pieces of software. The next step for this feature, since it is so new, is proper education and documentation so that people are using each environment as intended.

Version 0, edited 4 years ago by claytoncollie (next)

#21 @elrae
4 years ago

I'm not proposing we don't have a list of standard ones, I'm proposing we don't remove flexibility to change them. All plugins / hosts can have a switch statement and if they don't recognize my custom environment name, run the production mode. When you transfer hosts from WPEngine to another one they may or may not have the same environments, and they most definitely run different settings/code depending on that environment. What happens in the scenario above when I want my own environment variable so I can test debug on my plugin and run Yoast in production mode? I can't, I have to come up with my own new mechanism of defining an environment. That's where we are today, so what's different? Might as well rip this code out and tell all hosts to use the same environment names. If a developer calls their staging "stg" this was an intentional code level decision, and if they want the host they migrate to understand it they'll go into the code and change it. The worry here makes it seem as though changing these values is trivial or easily done in the admin; it's not. It's a developer choice and should be.

This ticket was mentioned in PR #503 on WordPress/wordpress-develop by jdevalk.


4 years ago
#22

  • Keywords has-patch added; needs-patch removed

Removes the ability to change the allowed environment types.

Trac ticket: https://core.trac.wordpress.org/ticket/50992

#23 @TimothyBlynJacobs
4 years ago

I think offline should be considered in a separate ticket for actually introducing an offline mode. I understand what offline means, but it seems better served by WP Airplane Mode. If we just add it as an allowed environment, but Core doesn't actually do the steps the plugin does it doesn't seem very helpful and would be cumbersome for plugins to adopt having to check for offline before making all their HTTP requests or loading external scripts etc...

I don't expect Yoast or any other plugin to know or care what my environment type is, and it shouldn't be doing anything different

@elrae It sounds like you are wanting more to identify the actual environment the site is on than the environment type. This feature was specifically introduced so plugins and WordPress Core can make decisions to alter their behavior based on the type of the environment.

I can't, I have to come up with my own new mechanism of defining an environment. That's where we are today, so what's different?

If you need the environment for your own purposes, than why not define your own constant and use that? What point is there of WordPress standardizing it if WordPress Core and Plugins can't rely on it?

All plugins / hosts can have a switch statement and if they don't recognize my custom environment name, run the production mode.

As discussed earlier in the ticket, that is of limited utility. If the custom environment name is closer to "development" then running as production doesn't make sense. But how can a plugin author or Core know whether the environment is closer to "development" or "staging" or "production" or "local"?

#25 @joostdevalk
4 years ago

I think offline should be considered in a separate ticket for actually introducing an offline mode.

agreed. took it out of my patch.

#26 @johnbillion
4 years ago

  • Owner set to johnbillion
  • Status changed from new to reviewing

#27 @SergeyBiryukov
4 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 48894:

Bootstrap/Load: Remove the ability to alter the list of environment types in wp_get_environment_type().

The intention of wp_get_environment_type() is to provide a consistent means of identifying the environment type, not of identifying a specific environment.

Actual environments should fit within one of the existing types: local, development, staging, or production. That should cover the types that plugins and themes might be concerned about when toggling functionality.

Props johnbillion, joostdevalk, TimothyBlynJacobs, jeremyfelt, batmoo, claytoncollie, Clorith, markjaquith, garrett-eclipse, GaryJ, elrae.
Fixes #50992.

#28 @SergeyBiryukov
4 years ago

In 48895:

Bootstrap/Load: Check if the __() function is available in wp_get_environment_type().

The function would not exist in SHORTINIT mode.

Follow-up to [48894].

See #50992.

#29 @SergeyBiryukov
4 years ago

In 48896:

Bootstrap/Load: Remove the ability to alter the list of environment types in wp_get_environment_type().

The intention of wp_get_environment_type() is to provide a consistent means of identifying the environment type, not of identifying a specific environment.

Actual environments should fit within one of the existing types: local, development, staging, or production. That should cover the types that plugins and themes might be concerned about when toggling functionality.

Props johnbillion, joostdevalk, TimothyBlynJacobs, jeremyfelt, batmoo, claytoncollie, Clorith, markjaquith, garrett-eclipse, GaryJ, elrae.
Merges [48894] and [48895] to the 5.5 branch.
Fixes #50992.

#30 @joostdevalk
4 years ago

  • Keywords has-dev-note added; needs-dev-note removed
Last edited 4 years ago by joostdevalk (previous) (diff)

#31 @giuseppe.mazzapica
4 years ago

Is not this a BC break change?

Besides agreeing or not with the matter, I can tell that in one of the websites that I'm working on to I have two environments, "prod-eu" and "prod-us" and based on
environment things like API keys changes. I used to manage those env vars on my own but decided to do the "WordPress way" for WP 5.5.

Now I need to change the workflow again. I can't use a database because the first check happens before WP is loaded, I'll probably revert back to my own environment management.

My point is that when support for a constant is added, it should not be removed in a point release that, as you know, might happen to be pushed overnight on environments via automatic updates.

I see you added a deprecation notice if WP_ENVIRONMENT_TYPES is defined, why not just keep this notice and wait at list next major to remove support?

Is not that the point of deprecation notices? First, you deprecate, in the next version you remove. Or am I wrong?

But I guess it's too late now.

Last edited 4 years ago by giuseppe.mazzapica (previous) (diff)
Note: See TracTickets for help on using tickets.