WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#36083 closed defect (bug) (fixed)

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

Reported by: joemcgill Owned by: joemcgill
Milestone: 4.5 Priority: high
Severity: major Version: 4.6
Component: Media Keywords: has-patch has-unit-tests commit
Focuses: administration Cc:

Description

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.

Attachments (4)

36083.patch (707 bytes) - added by joemcgill 3 years ago.
36083.diff (980 bytes) - added by swissspidy 3 years ago.
36083.2.diff (1.7 KB) - added by swissspidy 3 years ago.
36083.3.diff (2.3 KB) - added by swissspidy 3 years ago.

Download all attachments as: .zip

Change History (26)

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


3 years ago

#2 @joemcgill
3 years ago

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

#3 @joemcgill
3 years ago

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
3 years ago

#4 @joemcgill
3 years ago

  • Keywords has-patch 2nd-opinion added
  • Owner set to joemcgill
  • Status changed from new to accepted

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.

#5 @joemcgill
3 years ago

  • Milestone changed from Awaiting Review to 4.5

@swissspidy
3 years ago

#6 @swissspidy
3 years ago

  • Keywords has-unit-tests added

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.

#7 @azaozz
3 years ago

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
3 years ago

#8 @swissspidy
3 years ago

I like the name test_new_files_are_not_in_old_files_array().

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

#9 @joemcgill
3 years ago

  • Keywords commit added; 2nd-opinion removed

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.

#10 @ocean90
3 years ago

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.

#11 @swissspidy
3 years ago

@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.

#12 @joemcgill
3 years ago

@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
3 years ago

#13 @swissspidy
3 years ago

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.

#14 follow-up: @ocean90
3 years ago

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.

#15 in reply to: ↑ 14 @swissspidy
3 years ago

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.

#16 @SergeyBiryukov
3 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

#17 follow-up: @ocean90
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

'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.

#18 @ocean90
3 years ago

#36090 was marked as a duplicate.

#19 in reply to: ↑ 17 @joemcgill
3 years ago

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.

#20 @ocean90
3 years ago

In 36884:

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

The files are available since [36341].

See #36083, #35229.

#21 @ocean90
3 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

#22 @mikeschroder
3 years ago

#35575 was marked as a duplicate.

Note: See TracTickets for help on using tickets.