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

Migrate WordPress block asset files from PHP to JSON format #1656

Merged
merged 45 commits into from
Dec 19, 2023

Conversation

josephfusco
Copy link
Member

@josephfusco josephfusco commented Nov 15, 2023

Tasks

  • I have signed a Contributor License Agreement (CLA) with WP Engine.
  • If a code change, I have written testing instructions that the whole team & outside contributors can understand.
  • I have written and included a comprehensive changeset to properly document the changes I've made.

Description

These changes aim to fix the failing unzipping action reported in #1633.

Traditional WordPress blocks generate *.asset.php files. However, many hosting environments restrict PHP files in the uploads directory. This change proposes converting these files to index.asset.json, aligning with hosting constraints and improving compatibility.

Supporting tests have been added to both the cli package and WordPress plugin.

blockset-architecture

Related Issue(s):

#1522, #1633

Testing

  1. Checkout this branch: fix/blockset-issue-1633

  2. Update the block support example project (examples/next/block-support/) to be a WordPress site hosted on WP Engine.

  3. Add the updated faustwp.zip to your WordPress site.

  4. Run npm run blockset -w @faustwp/block-support-example from the root of this cloned repo.

  5. Observe "WordPress: Blockset sync complete" in your terminal.

    Screenshot 2023-11-17 at 11 11 29 AM
  6. Observe the blocks defined in your Next.js app have been compiled and are present in your connected WordPress site's uploads directory.

    Screenshot 2023-11-17 at 11 10 19 AM

Signed-off-by: Joe Fusco <[email protected]>
Copy link

changeset-bot bot commented Nov 15, 2023

🦋 Changeset detected

Latest commit: 42e0f85

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@faustwp/wordpress-plugin Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Signed-off-by: Joe Fusco <[email protected]>
Signed-off-by: Joe Fusco <[email protected]>
Signed-off-by: Joe Fusco <[email protected]>
@josephfusco josephfusco marked this pull request as ready for review November 15, 2023 16:59
@josephfusco josephfusco requested a review from a team as a code owner November 15, 2023 16:59
@wpengine wpengine deleted a comment from github-actions bot Nov 15, 2023
@wpengine wpengine deleted a comment from github-actions bot Nov 15, 2023
@theodesp
Copy link
Member

Hey @josephfusco thanks . Will review it today.

Signed-off-by: Joe Fusco <[email protected]>
Signed-off-by: Joe Fusco <[email protected]>
Signed-off-by: Joe Fusco <[email protected]>
Signed-off-by: Joe Fusco <[email protected]>
@wpengine wpengine deleted a comment from github-actions bot Nov 16, 2023
@wpengine wpengine deleted a comment from github-actions bot Nov 16, 2023
Signed-off-by: Joe Fusco <[email protected]>
Signed-off-by: Joe Fusco <[email protected]>
@wpengine wpengine deleted a comment from github-actions bot Nov 16, 2023
Signed-off-by: Joe Fusco <[email protected]>

# Conflicts:
#	examples/next/block-support/package.json
#	package-lock.json
Signed-off-by: Joe Fusco <[email protected]>
Signed-off-by: Joe Fusco <[email protected]>
@wpengine wpengine deleted a comment from github-actions bot Dec 1, 2023
@wpengine wpengine deleted a comment from github-actions bot Dec 1, 2023
@josephfusco josephfusco requested a review from mindctrl December 1, 2023 15:59
@josephfusco josephfusco force-pushed the fix/blockset-issue-1633 branch from df1b6f5 to 5cd554d Compare December 6, 2023 22:30
@wpengine wpengine deleted a comment from github-actions bot Dec 7, 2023
@josephfusco josephfusco removed their assignment Dec 7, 2023
@josephfusco josephfusco added the package: @faustwp/cli The issue relates to CLI package label Dec 8, 2023
@josephfusco josephfusco self-assigned this Dec 8, 2023
@theodesp
Copy link
Member

theodesp commented Dec 11, 2023

Hey @josephfusco . I was trying to test your changes and although I can see the uploaded files now both Local and WP Engine instance are throwing 500 errors. I believe its located when you read the block.json contents:

Screenshot 2023-12-11 at 13 32 21 Screenshot 2023-12-11 at 13 31 28

wp_remote_get throws error

Signed-off-by: Joe Fusco <[email protected]>
Signed-off-by: Joe Fusco <[email protected]>
@wpengine wpengine deleted a comment from github-actions bot Dec 13, 2023
@josephfusco josephfusco changed the title Improve unzipping compatibility Migrate WordPress block asset files from PHP to JSON format Dec 13, 2023
Copy link
Member

@theodesp theodesp left a comment

Choose a reason for hiding this comment

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

LGTM! I would be nice to have one more review

@mindctrl
Copy link
Contributor

mindctrl commented Dec 14, 2023

After syncing a block to my WPE site, I get this notice on every page load:

function register_block_script_handle was called incorrectly. The asset file (/wp-content/uploads/faustwp/blocks/block-a/index.asset.php) for the "editorScript" defined in "create-block/block-a" block definition is missing. (This message was added in version 5.5.0.)

Edit: also appears the blocks are not showing up in my WPE site, but do in a Local dev site.

@theodesp
Copy link
Member

After syncing a block to my WPE site, I get this notice on every page load:

function register_block_script_handle was called incorrectly. The asset file (/wp-content/uploads/faustwp/blocks/block-a/index.asset.php) for the "editorScript" defined in "create-block/block-a" block definition is missing. (This message was added in version 5.5.0.)

Edit: also appears the blocks are not showing up in my WPE site, but do in a Local dev site.

cc @josephfusco

Hey @mindctrl can you see the uploaded files in your WPE site?
I'm using WordPress 6.3.2 hosted in WPE site and I don't see any warnings.

Is there a specific config that you use?

@mindctrl
Copy link
Contributor

Hey @mindctrl can you see the uploaded files in your WPE site?
I'm using WordPress 6.3.2 hosted in WPE site and I don't see any warnings.

Is there a specific config that you use?

@theodesp having issues connecting to my site via SSH to see the folders, but will let you know soon. I just used the example code in this PR, nothing different or special locally.

@josephfusco
Copy link
Member Author

function register_block_script_handle was called incorrectly. The asset file (/wp-content/uploads/faustwp/blocks/block-a/index.asset.php) for the "editorScript" defined in "create-block/block-a" block definition is missing. (This message was added in version 5.5.0.)

I can confirm I'm seeing this warning too, although blocks are visible within the editor in both WP Engine & local environments.

@josephfusco
Copy link
Member Author

I've observed that blocks are functioning correctly in both the block editor and the GraphQL response, both in Local and WP Engine environments. This observation brings into question the relevance of the _doing_it_wrong warning that we're currently encountering.

The warning is triggered by the following line in the WordPress core: Asset File Requirement in blocks.php. This line enforces a .php file requirement for asset files.

In our case, however, dependencies are managed through a JSON file, which makes the .php requirement seem unnecessary. A screenshot illustrating the current warning is provided for reference:

Screenshot of the _doing_it_wrong warning

I propose introducing a filter at this point in the code. This filter would allow developers to inform WordPress that a .php file is not required in specific instances, such as ours where JSON is used for dependency management. This enhancement would provide greater flexibility in handling block dependencies, particularly for those of us leveraging modern development workflows.

@josephfusco
Copy link
Member Author

Proposed filter WordPress/wordpress-develop#5799

@josephfusco josephfusco merged commit 205fb09 into canary Dec 19, 2023
18 checks passed
@josephfusco josephfusco deleted the fix/blockset-issue-1633 branch December 19, 2023 20:31
@mindctrl mindctrl mentioned this pull request Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: @faustwp/cli The issue relates to CLI package package: faustwp Related to the companion WordPress plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Faust uploads missing block files with blockset command
4 participants