Make WordPress Core

Opened 7 years ago

Last modified 5 months ago

#47428 new defect (bug)

Site Health: Remove duplicate code in VCS test

Reported by: desrosj Owned by:
Priority: normal Milestone: Future Release
Component: Site Health Version: 5.2
Severity: normal Keywords: site-health has-patch has-unit-tests
Cc: Focuses:

Description

In WP_Site_Health_Auto_Updates::test_vcs_abspath(), almost the entire WP_Upgrader::is_vcs_checkout() function is duplicated. This test should be updated to use WP_Upgrader::is_vcs_checkout() instead. There are a few issues with this that would need to be solved, though.

In the message to the user, the directory under version control and the type of version control used are specified. This currently can't be determined without duplicating the logic. Also, in order to determine if the automatic_updates_is_vcs_checkout filter is overriding the function's result to allow updates, the filter would need to be run twice.

Props @johnbillion for the suggestion in #47388.

Change History (5)

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


7 years ago

#2 @SergeyBiryukov
7 years ago

  • Keywords site-health added

#3 @desrosj
7 years ago

  • Component AdministrationSite Health

Moving Site Health tickets into their lovely new home, the Site Health component.

#4 @Clorith
7 years ago

Avoiding duplication here would definitely be ideal, I'm not sure how we should approach the file/folder declarations though, as I view them as quite vital in the Site Health Check context, while it doesn't matter what is under VCS for WP_Upgrader.

Any thoughts on the best approach here?

Perhaps if WP_Upgrader::is_vcs_checkout() detects it being under version control, we can do a scan for .svn, .git, .hg and .bzr and pick up on their locations, RecursiveDirectoryIterator should be fast enough when given a rigid pattern like that to work with?

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


5 months ago
#5

  • Keywords has-patch has-unit-tests added

## Summary

Removes duplicated VCS checkout detection between Site Health and the automatic updater by centralizing the filesystem walk and .svn / .git / .hg / .bzr checks.

### Changes

  • Adds WP_Automatic_Updater::get_vcs_checkout_details() to return whether a VCS metadata directory was found, which ancestor path matched, and which marker (e.g. .git) was used—before the automatic_updates_is_vcs_checkout filter is applied.
  • Refactors WP_Automatic_Updater::is_vcs_checkout() to call get_vcs_checkout_details() and then apply the filter to the raw boolean, preserving existing behavior.
  • Updates WP_Site_Health_Auto_Updates::test_vcs_abspath() to use get_vcs_checkout_details( ABSPATH ) for the user-facing messages and applies automatic_updates_is_vcs_checkout once with the raw discovery result (equivalent to the previous logic when a checkout was detected).
  • Extends PHPUnit coverage in tests/phpunit/tests/admin/wpAutomaticUpdater.php for the new helper and consistency between raw discovery and the filtered is_vcs_checkout() result.

Trac ticket: https://core.trac.wordpress.org/ticket/47428

Note: See TracTickets for help on using tickets.