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

Fix is_decoupled() warning when frontend is running on different port #57

Merged
merged 5 commits into from
Aug 4, 2022

Conversation

alecgeatches
Copy link
Contributor

@alecgeatches alecgeatches commented Aug 1, 2022

I'm working on adding wp-env config to vip-go-nextjs-skeleton.

wp-env runs WordPress by default using localhost:8888, with the NextJS skeleton frontend running on localhost:3000. Since these are both on the same host, this triggers an admin warning from is_decoupled():

decoupled-warning

However, home_url() does point to a decoupled frontend: the frontend and backend are just running on different ports. This adds a small change to is_decoupled() to take the port into account before showing a warning and otherwise treating the site as improperly configured.

@alecgeatches alecgeatches requested a review from a team August 1, 2022 05:05
@alecgeatches alecgeatches self-assigned this Aug 1, 2022
@alecgeatches alecgeatches changed the title Fix Fix is_decoupled() warning when frontend is running on different port Aug 1, 2022
@alecgeatches
Copy link
Contributor Author

alecgeatches commented Aug 1, 2022

I'm looking into the failing unit tests:

Run composer test
> wp-env run phpunit 'phpunit -c /var/www/html/wp-content/plugins/vip-decoupled-bundle/phpunit.xml.dist'
ℹ Starting 'phpunit -c /var/www/html/wp-content/plugins/vip-decoupled-bundle/phpunit.xml.dist' on the phpunit container. 

Creating b9c477d7779e1fcb2081aab5b3817136_phpunit_run ... 
Creating b9c477d7779e1fcb2081aab5b3817136_phpunit_run ... done

wp_die() called
Message: <p>There has been a critical error on this website.</p><p><a href="https://wordpress.org/support/article/faq-troubleshooting/">Learn more about troubleshooting WordPress.</a></p>
Title: WordPress &rsaquo; Error
Args:
	response: 500
	code: internal_server_error
	exit: 
	back_link: 
	link_url: 
	link_text: 
	text_direction: ltr
	charset: UTF-8
	additional_errors: array (
)
255
✖ Command failed with exit code 255
Command failed with exit code 255
Script wp-env run phpunit 'phpunit -c /var/www/html/wp-content/plugins/vip-decoupled-bundle/phpunit.xml.dist' handling the test event returned with error code 1
Error: Process completed with exit code 1.

I can replicate the same error locally in trunk, but not sure why they're recently failing.

@alecgeatches
Copy link
Contributor Author

alecgeatches commented Aug 1, 2022

I can confirm the issue is related to the latest release 5.0.0 of wp-env. The error in the issue above can be resolved by downgrading to wp-env to 4.1.3.

Stepping through the code, the problem with 5.0.0 seems to be a WordPress version mismatch. In our test bootstrapping we load wp-phpunit's bootstrapping code here:

require "{$_tests_dir}/includes/bootstrap.php";

By stepping into that code locally in ~/.wp-env/{hash}/.../includes/bootstrap.php, I can see that wp-phpunit calls into install.php which tries to load wp-includes/class-wpdb.php which is not present in WordPress ^6.0. Upgrading wp-phpunit to the latest6.0.1 version also does not solve the problem. I think because the code is loaded from a container, not the local vendor folder, but I'm not sure.

This recent issue in Gutenberg WordPress/gutenberg#41780 is likely related. There has been an implementation change on how wp-phpunit code is loaded into wp-env which may require some configuration changes on our side to work correctly.

I'm looking into getting this solved to fix tests locally and for this repo.

@alecgeatches
Copy link
Contributor Author

alecgeatches commented Aug 2, 2022

Okay, I think this is fine to leave broken, as it'll resolve itself once WordPress/wordpress-develop@fdb6e13 makes it into a WordPress release. This is ready for review despite test failures.

Here's some more information on the problem and other solutions we could use:

Problem

This is why testing is currently broken:

  1. Since .wp-env.json doesn't have a "core" property set, the latest release version of WordPress (6.0.1) is used from a docker image. In this version of WordPress, the $wpdb include is located in /wp-includes/wp-db.php.

  2. Also since no core version is specified, wp-env fetches phpunit from trunk. In the trunk version of WordPress, wp-db.php has been renamed to class-wpdb.php.

  3. During installation of WordPress for testing, phpunit includes class-wpdb.php. Because class-wpdb.php does not exist on the latest release version of WordPress yet, the installation script fails and tests are not run.

Solutions

Hardcode WordPress version

This can be solved by specifying a core version in .wp-env.json:

{
	"plugins": [
		"."
	],
	"config": {},
+	"core": "WordPress/WordPress#6.0.1"
}

I can confirm that this fixes tests to run successfully in our repository. However, I don't believe we want to pin to a specific version and possibly start failing for legitimate reasons on newer versions of WordPress without knowing.

Change the way @wordpress/env pulls testing code

See related issue WordPress/gutenberg#41780 where this is also discussed. Ideally wp-env could pull tests in the same way as WordPress, which is to use the tag matching the latest release version of WordPress instead of using trunk directly. This would ensure that core and testing code align properly.

Currently, anytime there are breaking changes between WordPress versions we might see a similar mismatch between WordPress' latest release and WordPress' trunk test setup. I haven't investigated if this is a WIP yet, but I'd like to follow up on that after finishing up my interlude work. I still need to look into where WordPress source is being pulled. If it's from a docker container it may be tricky to align the two.

Wait until WordPress 6.1 is released

It seems like the wp-db.php/class-wpdb.php file rename is being released in 6.1. Once that's done, tests should automatically start working.


For the purposes of this PR, I'd like to leave tests failing since they're unrelated and not directly solvable in this repository without version pinning. Later, I'll look more into addressing the root version mismatch issue in @wordpress/env.

@chriszarate
Copy link
Member

Thanks for the deep investigation and write-up!

This can be solved by specifying a core version in .wp-env.json

I understand the drawbacks, but I'd recommend pursuing this strategy to fix tests, while opening a separate PR that reverts it, that is ready to merge once the issue is fixed upstream.

@alecgeatches
Copy link
Contributor Author

@chriszarate This is ready for another review!

I understand the drawbacks, but I'd recommend pursuing this strategy to fix tests, while opening a separate PR that reverts it, that is ready to merge once the issue is fixed upstream.

Good idea! I opened this PR which contains a revert commit for those changes.

I've also removed the wp-phpunit/wp-phpunit dependency in 1cdaa23 as it's no longer used. yoast/phpunit-polyfills is still required to run tests.

Copy link
Member

@chriszarate chriszarate left a comment

Choose a reason for hiding this comment

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

👍

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.

2 participants