WordPress.org

Make WordPress Core

Opened 20 months ago

Last modified 5 months ago

#49635 assigned enhancement

Build tools: Remove generated and package sourced files from committed code.

Reported by: peterwilsoncc Owned by: gziolo
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords:
Focuses: Cc:

Description

Currently several PHP files either auto-generated or copied during the build process are committed in to the repositories src directory, this includes:

  • src/wp-includes/assets/script-loader-packages.php
  • wp-includes/blocks/*.php

As including built files can lead to errors during the commit process, it would be dandy if these files could be removed from the repository and added to the list of ignored files.

A consideration is if this can be and needs to be done in such a way to avoid requiring JavaScript and CSS be built in order to run the PHP unit tests. I'm sure there are other complexities too.

Related #48154

Change History (12)

#1 @azaozz
20 months ago

I'm sure there are other complexities too.

Yeah, been thinking about this for a while. One problem is that once a file is added to SVN and committed, it never goes away completely. If it is deleted, it will not be present in the new(er) revisions, but it still exists in the older revisions from before it was deleted.

This causes a problem when a file is deleted from the repository and ignored, and then recreated when building. If a previous revision is restored, and there is already an ignored file with the same name at the same location, it will cause a conflict in SVN.

A way to work around this would be to rename the newly ignored files. But renaming files in /build (in production) may trigger fatal errors in some plugins... The other option is to just ignore these problems as they would happen relatively rarely and are usually pretty easy to fix.

Any other ideas/suggestions/thoughts? :)

#2 @bookdude13
20 months ago

As @azaozz mentioned in #48154 comment 67:

script-loader-packages.php is slightly different depending on whether Webpack was run in "build" or "dev" mode.

Thoughts on renaming that script-loader-packages-prod.php and script-loader-packages-dev.php to differentiate, and then .gitignore the current script-loader-packages.php? Not sure if that solves the underlying problem, but an idea.

This ticket was mentioned in Slack in #core-committers by sergey. View the logs.


17 months ago

#4 follow-up: @SergeyBiryukov
17 months ago

It was brought up in Slack that the wp-includes/assets/script-loader-packages.php file is:

As of [47471], I think it should be removed from the develop repo.

#5 in reply to: ↑ 4 @azaozz
17 months ago

Replying to SergeyBiryukov:

As of [47471], I think it should be removed from the develop repo.

Yes, wp-includes/assets/script-loader-packages.php can be SVN deleted, then added to svn:ignore and .gitignore. The same applies to wp-includes/blocks/*.php.

The problems (as mentioned in comment:1) are:

  1. These files will be missing or will not get updated in /src when doing npm run build or grunt build or similar. This might trigger a PHP fatal when testing but seems pretty rare edge case.
  2. After the delete + ignore steps are committed, reverting to an earlier revision (where the files are still under version control) may result in SVN conflicts that could also trigger PHP fatal(s) in /src if left unfixed.

Both of these seem like pretty rare edge cases, so can probably just "bite the bullet" and do it? :)

Last edited 17 months ago by azaozz (previous) (diff)

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


7 months ago

#7 @peterwilsoncc
7 months ago

  • Milestone changed from Awaiting Review to 5.8

Putting this on the 5.8 milestone for biting said bullet, the longer the issue remains the more problems it will cause.

For ease, I suspect this is best done just prior to branching so the two supported branches at the time are in sync without the files.

#8 @desrosj
7 months ago

Related: #50460.

#9 @gziolo
6 months ago

  • Owner set to gziolo
  • Status changed from new to assigned

@aristath tries to add individual block styles to optimize asset usage on the frontend for content created with blocks and it appears to become even more of an issue. We would have to keep track of 260 CSS files that are copied over and the process by webpack. Let's prioritize that. Related comment:
https://github.com/WordPress/wordpress-develop/pull/1236/files#r629091256

Last edited 6 months ago by gziolo (previous) (diff)

#10 follow-up: @gziolo
6 months ago

Removing everything from src/wp-includes/blocks folder fails 10 unit tests and causes 4 errors:

There were 4 errors:

1) WP_Test_Render_Reusable_Blocks::test_render
Error: Call to a member function render() on null

/var/www/tests/phpunit/tests/blocks/render-reusable.php:87

2) WP_Test_Render_Reusable_Blocks::test_render_subsequent
Error: Call to a member function render() on null

/var/www/tests/phpunit/tests/blocks/render-reusable.php:98

3) WP_Test_Render_Reusable_Blocks::test_ref_empty
Error: Call to a member function render() on null

/var/www/tests/phpunit/tests/blocks/render-reusable.php:125

4) WP_Test_Render_Reusable_Blocks::test_ref_wrong_post_type
Error: Call to a member function render() on null

/var/www/tests/phpunit/tests/blocks/render-reusable.php:131

--

There were 10 failures:

1) WP_Test_Render_Reusable_Blocks::test_recursive_render_warning
Failed asserting that exception of type "Error" matches expected exception "PHPUnit_Framework_Error_Warning". Message was: "Call to a member function render() on null" at
/var/www/tests/phpunit/tests/blocks/render-reusable.php:120
.

2) WP_Test_Block_Render::test_do_blocks_removes_comments
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
 
 <p>Third Auto Paragraph</p>
 
-<p>[someshortcode]</p>
-<p>And some content?!</p>
-<p>[/someshortcode]</p>
+
+[someshortcode]
+
+And some content?!
+
+[/someshortcode]
 
 '

/var/www/tests/phpunit/includes/abstract-testcase.php:668
/var/www/tests/phpunit/tests/blocks/render.php:63

3) WP_Test_Block_Render::test_the_content
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
 '<p>Foo</p>
-</p>
-<p>Bar</p>
-<p>
+Bar
 <p>Baz</p>'

/var/www/tests/phpunit/tests/blocks/render.php:83

4) WP_Test_Block_Render::test_do_block_output with data set #1 ('core__archives.html', 'core__archives.server.html')
File '/var/www/tests/phpunit/includes/../data/blocks/fixtures/core__archives.html' does not match expected value
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<div class=" wp-block-archives-list wp-block-archives">No archives to show.</div>'
+''

/var/www/tests/phpunit/tests/blocks/render.php:228

5) WP_Test_Block_Render::test_do_block_output with data set #2 ('core__archives__showPostCounts.html', 'core__archives__showPostCount...r.html')
File '/var/www/tests/phpunit/includes/../data/blocks/fixtures/core__archives__showPostCounts.html' does not match expected value
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<div class=" wp-block-archives-list wp-block-archives">No archives to show.</div>'
+''

/var/www/tests/phpunit/tests/blocks/render.php:228

6) WP_Test_Block_Render::test_do_block_output with data set #6 ('core__categories.html', 'core__categories.server.html')
File '/var/www/tests/phpunit/includes/../data/blocks/fixtures/core__categories.html' does not match expected value
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<ul class="wp-block-categories-list wp-block-categories"><li class="cat-item-none">No categories</li></ul>
+'
 '

/var/www/tests/phpunit/tests/blocks/render.php:228

7) WP_Test_Block_Render::test_do_block_output with data set #33 ('core__latest-comments.html', 'core__latest-comments.server.html')
File '/var/www/tests/phpunit/includes/../data/blocks/fixtures/core__latest-comments.html' does not match expected value
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<div class="has-avatars has-dates has-excerpts no-comments wp-block-latest-comments">No comments to show.</div>
+'
 '

/var/www/tests/phpunit/tests/blocks/render.php:228

8) WP_Test_Block_Render::test_do_block_output with data set #34 ('core__latest-posts.html', 'core__latest-posts.server.html')
File '/var/www/tests/phpunit/includes/../data/blocks/fixtures/core__latest-posts.html' does not match expected value
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<ul class="wp-block-latest-posts__list wp-block-latest-posts"></ul>
+'
 '

/var/www/tests/phpunit/tests/blocks/render.php:228

9) WP_Test_Block_Render::test_do_block_output with data set #35 ('core__latest-posts__displayPo...e.html', 'core__latest-posts__displayPo...r.html')
File '/var/www/tests/phpunit/includes/../data/blocks/fixtures/core__latest-posts__displayPostDate.html' does not match expected value
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<ul class="wp-block-latest-posts__list has-dates wp-block-latest-posts"></ul>
+'
 '

/var/www/tests/phpunit/tests/blocks/render.php:228

10) WP_Test_Block_Render::test_do_block_output with data set #54 ('core__shortcode.html', 'core__shortcode.server.html')
File '/var/www/tests/phpunit/includes/../data/blocks/fixtures/core__shortcode.html' does not match expected value
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<p>[gallery ids="238,338"]</p>
+'
+[gallery ids="238,338"]
 
 '

/var/www/tests/phpunit/tests/blocks/render.php:228

ERRORS!
Tests: 12222, Assertions: 56287, Errors: 4, Failures: 10, Skipped: 15.

Adding PHP files back doesn't change anything because those tests depend on registered core blocks that use block.json files that we would like to keep out of svn tracking. As far as I can tell, we don't run npm run build:dev when executing GitHub workflows so this would be an issue there as well.

I expect the same set of challenges when we remove wp-includes/assets/script-loader-packages.php from svn.

Should we introduce some sort of integration testing that would run after npm run build:dev to perform some basic checks on whether the block editor gets updated correctly?

#11 in reply to: ↑ 10 @johnbillion
5 months ago

Replying to gziolo:

Should we introduce some sort of integration testing that would run after npm run build:dev to perform some basic checks on whether the block editor gets updated correctly?

We've got a few already for the tests that were moved out of PHPUnit tests in [50441]: https://github.com/WordPress/wordpress-develop/blob/0a3a3c5119897c6d551a42ae9b5dbfa4f576f2c9/Gruntfile.js#L1458-L1465

#12 @desrosj
5 months ago

  • Milestone changed from 5.8 to 5.9

I'm going to punt this and #50460 to 5.9 as they're not quite ready, and today is feature freeze.

Test and build tool changes are allowed after feature freeze up until RC, so if someone is able to work on this before then, it can always be moved back.

Note: See TracTickets for help on using tickets.