Automattic / vip-decoupled-bundle

WordPress VIP decoupled plugin bundle
29 stars 5 forks source link

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

Closed alecgeatches closed 2 years ago

alecgeatches commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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:

https://github.com/Automattic/vip-decoupled-bundle/blob/d334dd1709e8e53c8ca501b4104f6f240149a999/tests/bootstrap.php#L51

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 https://github.com/WordPress/gutenberg/pull/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 commented 2 years ago

Okay, I think this is fine to leave broken, as it'll resolve itself once https://github.com/WordPress/wordpress-develop/commit/fdb6e13fedc46fc19852479bad2488c9eab1ed9f 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 https://github.com/WordPress/gutenberg/pull/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 commented 2 years ago

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 commented 2 years ago

@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 https://github.com/Automattic/vip-decoupled-bundle/pull/57/commits/1cdaa23cfd1f8f71300cd953400b3d4ba34ee3ac as it's no longer used. yoast/phpunit-polyfills is still required to run tests.