Make WordPress Core

Opened 9 months ago

Closed 5 months ago

#64221 closed task (blessed) (fixed)

Reclassify `json2php` as a `devDependency`

Reported by: desrosj Owned by: desrosj
Priority: normal Milestone: 7.0
Component: Build/Test Tools Version:
Severity: normal Keywords: good-first-bug has-patch changes-requested commit
Cc: Focuses: tests

Description

It looks like json2php is included in the list of dependencies, but it appears the package is only used within the copy:block-json Grunt task to convert block.json file contents to a block-json.php equivalent.

The package is even explicitly excluded from the unit test that asserts that all dependencies are registered and available as expected.

Change History (12)

This ticket was mentioned in PR #10520 on WordPress/wordpress-develop by @royhridoy.


9 months ago
#1

  • Keywords has-patch added; needs-patch removed

The json2php package is only used by the build process and is not required at runtime. It is even explicitly excluded from the runtime dependency unit test.

This change moves json2php from dependencies to devDependencies and regenerates the lockfile.

Trac ticket: Core-64221

#2 @SergeyBiryukov
9 months ago

  • Milestone Awaiting Review7.0

#3 @westonruter
9 months ago

  • Owner set to desrosj
  • Status newreviewing

#4 @noruzzaman
7 months ago

Test Report

Description

This report validates whether the indicated patch works as expected.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/10520

Environment

  • WordPress: 7.0.0-alpha (trunk)
  • PHP: 8.2.x (Local environment)
  • Server: nginx (Local environment)
  • Database: mysqli
  • Browser: Chrome
  • OS: macOS 15.2
  • Theme: Twenty Twenty-Five (Default)
  • MU Plugins: None activated
  • Plugins: None

Actual Results

✅ json2php is correctly moved from dependencies to devDependencies in package.json.
✅ npm install successfully updates package-lock.json.
✅ npm run build completes without any errors.
✅ npx grunt copy:block-json task executes successfully, confirming the dependency is functional as a devDependency.

kmgalanakis commented on PR #10520:


7 months ago
#5

Hi @RoyHridoy

I've reviewed the code changes, and everything appears to be in order. Packages are getting installed properly, the build completes without any errors, and tests pass locally.

This is good to go.

Thank you

@phpbits commented on PR #10520:


6 months ago
#6

Hi @RoyHridoy,

I also tested this branch, and the packages are installing correctly. I'm encountering these test errors locally, which may not be related to the changes here.

There were 86 warnings:

1) Tests_Actions::test_priority_callback_order with data set "float DESC" (array(10.0, 9.5), array('action2', 'action'), 'Implicit conversion from floa...cision')
Expecting E_DEPRECATED and E_USER_DEPRECATED is deprecated and will no longer be possible in PHPUnit 10.

/var/www/vendor/bin/phpunit:122

2) Tests_Actions::test_priority_callback_order with data set "float ASC" (array(9.5, 10.0), array('action', 'action2'), 'Implicit conversion from floa...cision')
Expecting E_DEPRECATED and E_USER_DEPRECATED is deprecated and will no longer be possible in PHPUnit 10.

/var/www/vendor/bin/phpunit:122

3) Tests_Admin_IncludesPost::test_post_exists_should_support_post_type
Expecting E_DEPRECATED and E_USER_DEPRECATED is deprecated and will no longer be possible in PHPUnit 10.

/var/www/vendor/bin/phpunit:122

4) Tests_Admin_IncludesPost::test_post_exists_should_not_match_a_page_for_post
Expecting E_DEPRECATED and E_USER_DEPRECATED is deprecated and will no longer be possible in PHPUnit 10.

/var/www/vendor/bin/phpunit:122

5) Tests_Admin_IncludesPost::test_post_exists_should_support_post_status
Expecting E_DEPRECATED and E_USER_DEPRECATED is deprecated and will no longer be possible in PHPUnit 10.

/var/www/vendor/bin/phpunit:122

#7 @dilip2615
6 months ago

  • Focuses tests added

Hello @desrosj, I reviewed the patch in PR #10520, and it looks correct. json2php is only used in the build tooling (copy:block-json) and not needed at runtime, so moving it from dependencies to devDependencies makes sense.

Tested locally:

  • npm install (lockfile updated as expected)
  • npm run build (no errors)
  • npx grunt copy:block-json (task runs successfully). => +1 to commit/merge. Any PHPUnit deprecation warnings I saw appear unrelated to this change.

#8 @desrosj
6 months ago

  • Keywords changes-requested added

@dilip2615 are you still seeing the copy:block-json script locally? That Grunt script was removed in [61438], so I'm not sure how npx grunt copy:block-json did not return an error for you in your testing.

The new build script still depends on json2php, so moving to devDependency does still make sense.

The PR has a merge conflict, though (likely caused by [61438]), so that needs to be refreshed in order to retest.

This ticket was mentioned in PR #10977 on WordPress/wordpress-develop by @nimeshatxecurify.


6 months ago
#9

Refresh

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


6 months ago

#11 @desrosj
5 months ago

  • Keywords commit added

#12 @desrosj
5 months ago

  • Resolutionfixed
  • Status reviewingclosed

In 62061:

Build/Test Tools: Reclassify json2php as a devDependency.

This dependency is responsible for transforming some JSON files to PHP ones. But since this is not included in the distributed software, it should not be specified as a dependency.

Props nimeshatxecurify, royhridoy, phpbits, dilip2615, noruzzaman.
Fixes #64221.

Note: See TracTickets for help on using tickets.