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

[peterwilsoncc]

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

[azaozz]

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? :)

[bookdude13]

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.

[SergeyBiryukov]

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

• Up-to-date in the build repo: ​https://build.trac.wordpress.org/browser/trunk/wp-includes/assets
• Outdated in the develop repo: https://core.trac.wordpress.org/browser/trunk/src/wp-includes/assets

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

[azaozz]

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? :)

[peterwilsoncc]

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.

[desrosj]

Related: #50460.

[gziolo]

@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

[gziolo]

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?

[johnbillion]

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

[desrosj]

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.

[hellofromTonya]

Well boo, it didn't get attention during the 5.9 cycle either. Today is Feature Freeze.

Like before, I'll punt this and #50460 to Future Release (as 6.0 isn't available yet for selection).

As @desrosj noted last time, 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.