Media Grid: 'wp-admin/upload.php' broken in nightly builds

[joemcgill]

The media library view 'wp-admin/uploads.php' is not displaying properly in the nightly builds. It looks like the HTML is being rendered properly but there are missing CSS rules causing the problem.

To reproduce:

• Create a fresh install of WP
• Add a few images to your media library
• Update to latest nightly build (using beta tester plugin)
• See the media library disappear on 'wp-admin/upload.php'

Currently broken on (4.5-beta1-36808 but also yesterday).

Interestingly, I can't reproduce this by building from source using grunt build, but I do see 938 lines of CSS missing from 'load-styles.php' on that page (with identical query vars), which is likely the cause.

As an aside, it looks like some styles kick in that can make the grid appear when the viewport is shortened below 400px (see screencap: ​https://cloudup.com/iM4VzBnpfM5). I don't think that this is a problem, but can be confusing when debugging.

Originally reported in the alpha/beta support forums: ​see thread.

[joemcgill]

Update: looks like this was reported in Slack on Feb 28 ​view logs.

[joemcgill]

Looks like the updater plugin is removing the wp-admin/css/media.min.css file. Copying from a fresh download of wordpress-latest.zip fixes the issue.

[joemcgill]

36083.patch​ comments out the missing minified files being removed in update-core.php via the $_old_files array and notes that these files were added back in 4.5.

[swissspidy]

36083.diff​ adds a unit test to ensure this never happens again. Since RTL and minified files are in the list, but only source files are in trunk, I added multiple assertions.

[azaozz]

Both the patch and the unit test look good. Perhaps the test can be renamed from test_old_files_should_not_exist() to something like test_new_files_are_not_in_old_files_array().

[swissspidy]

I like the name test_new_files_are_not_in_old_files_array().

36083.2.diff​ is a combination of the two patch files.

[joemcgill]

I'm happy with that test name. Since it's a unit test, we don't have to worry nearly as much about naming from a back-compat standpoint so long as it is descriptive of what's happening when it fails.

[ocean90]

36083.2.diff​ wouldn't have catch this issue, right? IMO the test makes only sense if it tests the files in /build. See trunk/tests/phpunit/tests/oembed/template.php?rev=36694&marks=297-301#L290​ for another test for /build.

[swissspidy]

@ocean90 It would, but only because of the str_replace() "hack". I can change the test to check inside /build if it is more reliable.

[joemcgill]

@ocean90 I tested 36083.diff​ with and without my original patch applied and it did catch the error. The tests pass once 36083.patch​ is applied. Just to be sure, I checked my wp-tests-config.php file and am testing the src directory, i.e., define( 'ABSPATH', dirname( __FILE__ ) . '/src/' );

[swissspidy]

Just to make sure, 36083.3.diff​ adds a new test for the /build directory.

Try it yourself, when adding back the commented out lines both tests fail.

[ocean90]

Sorry, it took a few minutes to understand what the test is testing. :-)

So yeah, it's working but IMO it's testing the wrong assumption: Just because there is a file.css it doesn't mean that there must be file.min.css, file-rtl.css and file-rtl.min.css.
customize-preview.css is a good example because it doesn't need a RTL version.

Farbtastic is interesting because we don't use the minified file currently, see trunk/src/wp-includes/script-loader.php@36806#L778​. The $suffix is missing.

[swissspidy]

Replying to ocean90:

Sorry, it took a few minutes to understand what the test is testing. :-)

So yeah, it's working but IMO it's testing the wrong assumption: Just because there is a file.css it doesn't mean that there must be file.min.css, file-rtl.css and file-rtl.min.css.

It's testing that there are no such files, not that they must exist.

[SergeyBiryukov]

In 36843:

Comment out some CSS files in $_old_files that were added back as a result of [36341].

Add a unit test to make sure the $_old_files array does not contain any current project files.

Props joemcgill, swissspidy.
Fixes #36083.

[ocean90]

'wp-admin/css/farbtastic-rtl.min.css' wasn't added in 4.5, it doesn't get used at all.

It's testing that there are no such files, not that they must exist.

Yes and no, add wp-includes/css/customize-preview-rtl.min.css to $_old_files as an example. Use case: We have changed a file which doesn't require an extra RTL stylesheet anymore, so we add the file to $_old_files.

Also, test_new_files_are_not_in_old_files_array doesn't include *-rtl.css files. I think the test_new_files_are_not_in_old_files_array_compiled is the only test we need for this. It uses only 646 file_exists() checks while the other does has three for each file.

[ocean90]

#36090 was marked as a duplicate.

[joemcgill]

Replying to ocean90:

'wp-admin/css/farbtastic-rtl.min.css' wasn't added in 4.5, it doesn't get used at all.

'wp-admin/css/farbtastic-rtl.min.css' is automatically included in nightly builds but will be removed during updates. If it's not in use any more shouldn't we remove this file in our builds as well? Seems we would be better off trying to eliminate differences between core files remaining after an update and files delivered in a clean build.

[ocean90]

In 36884:

Script Loader: Enqueue the minified version of farbtastic if SCRIPT_DEBUG is false.

The files are available since [36341].

See #36083, #35229.

[ocean90]

In 36885:

Tests: Remove test_new_files_are_not_in_old_files_array() from [36843].

The test assumes that if a CSS file was added to $_old_files all three files (.css, .min.css, -rtl.min.css; it's actually missing the fourth case, -rtl.css) don't exist anymore. But this isn't always the case. The test is also incredible slow because it does three file_exists() checks for each file — the global contains 646 files currently.

It's important what we have in the /build directory and that's covered by test_new_files_are_not_in_old_files_array_compiled().

Fixes #36083.

[kirasong]

#35575 was marked as a duplicate.