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

[BUGS-7678] WP config HTTP_HOST documentation updates #8969

Merged
merged 19 commits into from
May 7, 2024

Conversation

jazzsequence
Copy link
Contributor

@jazzsequence jazzsequence commented Apr 26, 2024

Summary

Remaining Work and Prerequisites

Release:

  • When ready
  • After date: $DATE

Post Launch

Do not remove - To be completed by the docs team upon merge:

  • Redirect /old-path/ => /new-path/ (if applicable)
  • Include/exclude pages ^ respectively within docs search service provider (if applicable)

@jazzsequence jazzsequence requested review from a team as code owners April 26, 2024 01:43
Copy link

⚡ Deployed with Pantheon Decoupled

This build was successfully deployed with Pantheon. You can track the build logs here.

👀 Preview: https://pr-8969-documentation.appa.pantheon.site
🛠️ Manage in Pantheon: https://dashboard.pantheon.io/site/2b30153f-e8b1-4427-b076-6109e704ba5d/overview

Copy link
Contributor

@stevector stevector left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jazzsequence could we do something in a plugin that made copy/pasting a big switch statement unnecessary? That might be too big a request for this PR.

source/content/guides/multisite/03-config.md Outdated Show resolved Hide resolved
source/content/guides/multisite/03-config.md Outdated Show resolved Hide resolved
@pwtyler
Copy link
Member

pwtyler commented Apr 26, 2024

could we do something in a plugin that made copy/pasting a big switch statement unnecessary? That might be too big a request for this PR.

I would love to see a version of this automated as part of the site launch workflow for wpms sites

@jazzsequence
Copy link
Contributor Author

jazzsequence commented Apr 26, 2024

@stevector

could we do something in a plugin that made copy/pasting a big switch statement unnecessary? That might be too big a request for this PR.

the fundamental ask here is that there needs to be a fallback domain that is customer-specific and not reliant on server variables. We could simplify this somewhat and remove the switch, but there's every chance that it will break things because we have different environments that need different rules (and using $_SERVER['HTTP_HOST'] was the way around that). We can't add something like a single filter or something that could be used in code and remove all the other stuff because this is before WordPress execution even starts, so there's nothing to hook into.

Ultimately, the configuration file (be it config/application.php or WP Composer Managed or wp-config.php) is customer code that we can't make changes to without breaking their sites. The change has to come from the user, at least as far as the fallback hostname is concerned. As @pwtyler mentions, we can build something new into a workflow for creating WordPress multisites specifically, but that would likely be a whole new site creation workflow (e.g. Create New Site -> WordPress -> WordPress Multisite) that doesn't exist today.

whether or not the full superglobal is available is somewhat irrelevant, we can just talk about the value we actually care about, which is HTTP_HOST
Copy link

⚡ Deployed with Pantheon Decoupled

This build was successfully deployed with Pantheon. You can track the build logs here.

👀 Preview: https://pr-8969-documentation.appa.pantheon.site
🛠️ Manage in Pantheon: https://dashboard.pantheon.io/site/2b30153f-e8b1-4427-b076-6109e704ba5d/overview

@jazzsequence jazzsequence requested a review from stevector April 26, 2024 21:41
Copy link

⚡ Deployed with Pantheon Decoupled

This build was successfully deployed with Pantheon. You can track the build logs here.

👀 Preview: https://pr-8969-documentation.appa.pantheon.site
🛠️ Manage in Pantheon: https://dashboard.pantheon.io/site/2b30153f-e8b1-4427-b076-6109e704ba5d/overview

Copy link

⚡ Deployed with Pantheon Decoupled

This build was successfully deployed with Pantheon. You can track the build logs here.

👀 Preview: https://pr-8969-documentation.appa.pantheon.site
🛠️ Manage in Pantheon: https://dashboard.pantheon.io/site/2b30153f-e8b1-4427-b076-6109e704ba5d/overview

@stevector
Copy link
Contributor

Ultimately, the configuration file (be it config/application.php or WP Composer Managed or wp-config.php) is customer code that we can't make changes to without breaking their sites.

What about hiding some of this complexity in wp-config-pantheon.php ? Even declaring a function there (or in prepend.php) that simplified the setting of these variables would be better than this much copy/pasting.

@jazzsequence
Copy link
Contributor Author

jazzsequence commented Apr 29, 2024

What about hiding some of this complexity in wp-config-pantheon.php ? Even declaring a function there (or in prepend.php) that simplified the setting of these variables would be better than this much copy/pasting.

The problem is this is in the mu-plugin. Specifically, it's in the instructions for setting up WordPress multisite that displays in the admin when you are setting up a multisite. We are making a change in the MU Plugin already with pantheon-systems/pantheon-mu-plugin#42. This PR is updating our documentation so it matches the guidance that's already in the mu-plugin. This is something that people are already copy/pasting.

@jazzsequence
Copy link
Contributor Author

jazzsequence commented Apr 29, 2024

MU plugins are loaded after wp-config.php, so there's nothing we could put into the MU plugin that customer code could expect (e.g. if we wanted to define a constant in the MU plugin and have customers use that as a fallback in their wp-config.php).

HOWEVER, it is conceivable that we could define a constant in wp-config-pantheon.php and config/application-pantheon.php (for Composer Managed) that customers could override in their wp-config.php. This is code that I'm playing with...

Pantheon-maintained code (would exist in the WordPress upstream)

if ( ! defined( 'PANTHEON_HOSTNAME' ) ) {
    $site_name = $_ENV['PANTHEON_SITE_NAME'];
    $hostname = ! isset( $_SERVER['HTTP_HOST'] ) ? $_ENV['PANTHEON_ENVIRONMENT'] . "-$site_name.pantheonsite.io" : $_SERVER['HTTP_HOST'];
    define( 'PANTHEON_HOSTNAME', $hostname );
}

This would be the customer code (default):

define('DOMAIN_CURRENT_SITE', PANTHEON_HOSTNAME);

However, they could customize it like this if they wanted:

define( 'PANTHEON_HOSTNAME', [
	'dev' => 'pantheondev.chrisreynolds.io',
	'test' => 'pantheontest.chrisreynolds.io',
	'live' => 'pantheon.chrisreynolds.io',	
] );
$hostname = isset( $_SERVER['HTTP_HOST'] ) ? $_SERVER['HTTP_HOST'] : PANTHEON_HOSTNAME[ $_ENV['PANTHEON_ENVIRONMENT'] ];
define('MULTISITE', true);
define('SUBDOMAIN_INSTALL', false);
define('DOMAIN_CURRENT_SITE', $hostname);
define('PATH_CURRENT_SITE', '/');
define('SITE_ID_CURRENT_SITE', 1);
define('BLOG_ID_CURRENT_SITE', 1);

This is a change in the approach because we'd need to make upstream changes, so this is a @scottbuscemi Product decision.

@jazzsequence
Copy link
Contributor Author

Discussed in PBR. We're going to try the alternative approach I outline in my last comment. tl:dr; this will mean essentially one (optional) change to the customer's wp-config.php file (or config/application.php if using Bedrock/Composer) for multisite and we can remove the switches entirely. I need to do additional testing to ensure it works for all the things, but I will move this PR back to draft while I'm working this angle.

@jazzsequence jazzsequence marked this pull request as draft April 29, 2024 19:02
@jazzsequence jazzsequence dismissed stevector’s stale review April 29, 2024 19:02

rethinking the approach, moving docs PR back to draft

@stevector
Copy link
Contributor

Thanks @jazzsequence!

Copy link

⚡ Deployed with Pantheon Decoupled

This build was successfully deployed with Pantheon. You can track the build logs here.

👀 Preview: https://pr-8969-documentation.appa.pantheon.site
🛠️ Manage in Pantheon: https://dashboard.pantheon.io/site/2b30153f-e8b1-4427-b076-6109e704ba5d/overview

Copy link

⚡ Deployed with Pantheon Decoupled

This build was successfully deployed with Pantheon. You can track the build logs here.

👀 Preview: https://pr-8969-documentation.appa.pantheon.site
🛠️ Manage in Pantheon: https://dashboard.pantheon.io/site/2b30153f-e8b1-4427-b076-6109e704ba5d/overview

Copy link

⚡ Deployed with Pantheon Decoupled

This build was successfully deployed with Pantheon. You can track the build logs here.

👀 Preview: https://pr-8969-documentation.appa.pantheon.site
🛠️ Manage in Pantheon: https://dashboard.pantheon.io/site/2b30153f-e8b1-4427-b076-6109e704ba5d/overview

@jazzsequence jazzsequence marked this pull request as ready for review April 29, 2024 21:20
@jazzsequence jazzsequence requested a review from pwtyler April 29, 2024 21:20
Copy link

⚡ Deployed with Pantheon Decoupled

This build was successfully deployed with Pantheon. You can track the build logs here.

👀 Preview: https://pr-8969-documentation.appa.pantheon.site
🛠️ Manage in Pantheon: https://dashboard.pantheon.io/site/2b30153f-e8b1-4427-b076-6109e704ba5d/overview

Copy link

⚡ Deployed with Pantheon Decoupled

This build was successfully deployed with Pantheon. You can track the build logs here.

👀 Preview: https://pr-8969-documentation.appa.pantheon.site
🛠️ Manage in Pantheon: https://dashboard.pantheon.io/site/2b30153f-e8b1-4427-b076-6109e704ba5d/overview

if `$_ENV['PANTHEON_ENVIRONMENT']` is unset (e.g. in non-Lando local development environments), using PANTHEON_HOSTNAME will fatal as undefined, so we still need a fallback here.
Copy link

⚡ Deployed with Pantheon Decoupled

This build was successfully deployed with Pantheon. You can track the build logs here.

👀 Preview: https://pr-8969-documentation.appa.pantheon.site
🛠️ Manage in Pantheon: https://dashboard.pantheon.io/site/2b30153f-e8b1-4427-b076-6109e704ba5d/overview

@jazzsequence
Copy link
Contributor Author

jazzsequence commented Apr 30, 2024

We've merged everything, but we're going to hold off on releasing docs until the next WordPress update goes out and pushes the https://github.com/pantheon-systems/WordPress upstream update out with it.

Release note will be updated at that time for WordPress update.

@jazzsequence jazzsequence merged commit 1c2e745 into main May 7, 2024
8 checks passed
@jazzsequence jazzsequence deleted the bugs-7678-http_host-docs branch May 7, 2024 19:58
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.

4 participants