Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#55712 closed enhancement (fixed)

Remove _copy_dir() as originally intended

Reported by: afragen's profile afragen Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.1 Priority: normal
Severity: normal Version: 3.2
Component: Upgrade/Install Keywords: has-patch commit
Focuses: Cc:

Description

This is a follow-up to #17173.

Per #17173, the original intent was to remove _copy_dir() after the WP 3.1 -> 3.2 update. Apparently there were issues with the Updates API 11 years ago that likely don't exist today.

Opening this ticket as the original is on a completed milestone.

Change History (12)

This ticket was mentioned in PR #2703 on WordPress/wordpress-develop by afragen.


2 years ago
#1

As originally described, adding _copy_dir() was supposed to be a short term fix for updating 3.1 -> 3.2 installations. I think it's time and we can clean up some duplicate code. I believe the original issue and objections have been sorted out in the Updates API.

Trac ticket: [ https://core.trac.wordpress.org/ticket/55712 #55712], #17173

#2 @afragen
2 years ago

Original ticket #17173

Ping @dd32

#3 @costdev
2 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 6.1
  • Type changed from defect (bug) to enhancement
  • Version set to 3.2

This looks like a clean change.

Regarding BC:

  • This is a private function marked with @private.
  • Public extender usage shows one instance, which is behind a function_exists() guard, and in a plugin that is now closed on the repository.

I added the PR's changes to a local install and was able to upgrade to 6.0-RC2 without issue. After searching for unit tests for this function, it looks like there's none to remove.

Approved the PR and it looks good for commit consideration.

#4 @dd32
2 years ago

Looks good to me too.

#5 @peterwilsoncc
2 years ago

Expanding on any back-compat concerns, to access the function does require a require or include.

wp> function_exists( 'copy_dir' );
bool(true)

wp> function_exists( '_copy_dir' );
bool(false)

Additionally the function does not appear in the developer documentation due to an @ignore directive.

There is still a part of me tempted to deprecate the function in place though. It's not a lot of effort to avoid fatal errors:

<?php
function _copy_dir( $from, $to, $skip_list = array() ) {
        _deprecated_function( __FUNCTION__, 'copy_dir' );
        return copy_dir( $from, $to, $skip_list ); /* optionally */
}

#6 @costdev
2 years ago

Scroll to the bottom for the TL;DR.

Search 1
As linked above, I searched wpdirectory.net for public plugins and themes referencing _copy_dir\( (with a leading space).

  • There was one result, a plugin, which called the function.
  • The call was protected by a function_exists() guard.
  • It was submitted 11 years ago.
  • It was a plugin for unit testing.
  • This plugin is now closed.
  • Its version was 0.0.2.

Search 2
I searched Google for _copy_dir: No codebase or developer discussions on this.


Search 3
I searched GitHub for _copy_dir with:

  • The language set to PHP.
  • The extension set to php.
  • Search *not* in forks set.

The first 20 pages, the last 10 pages, and the middle 10 pages are all from wp-admin/includes/update-core.php stored in the repositories. I can't find any usage of it anywhere, other than from the 11 year old plugin mentioned in the results in my earlier comment.


Search 4
I ran this search on wpdirectory.net (which accounts for different coding styles) for plugins that have:

  1. A require/require_once/include/include_once statement for wp-admin/includes/update-core.php.
  2. A reference just to wp-admin/includes/update-core.php.

After reviewing all 53 results:

  1. These are used either for calling update_core(), or making use of the $_old_files array.
  2. These are references to the file, and don't concern me.

Overview

  • Doesn't appear in the developer documentation.
  • Is marked with a leading underscore.
  • Needs a require or include statement in order to be used.
  • Has an alternative via copy_dir(), which was introduced earlier (2.5.0), is public, is available in developer documentation and can be found in references online.
  • Available sources that do have a require/require_once/include/include_once statement for the file containing this function are only looking at update_core() and the $_old_files array.
  • Has one ancient, now defunct, reference to it online (from 11 years ago, which is when this function was introduced).

I think this is just about as safe as anyone could expect when removing a function from Core, and can't find any indication that deprecating it would be of any benefit here.

Version 2, edited 2 years ago by costdev (previous) (next) (diff)

#7 in reply to: ↑ description @SergeyBiryukov
2 years ago

Replying to afragen:

Per #17173, the original intent was to remove _copy_dir() after the WP 3.1 -> 3.2 update. Apparently there were issues with the Updates API 11 years ago that likely don't exist today.

I think this could use some further clarification on what exactly the issues were, and whether they indeed no longer exist. Looking at the discussions in comment:24:ticket:14484 and comment:7:ticket:17173, it seems like this had to do with Akismet getting reinstalled upon update.

comment:9:ticket:17173 could also use some investigation to confirm that the current API handles this as expected.

Last edited 2 years ago by SergeyBiryukov (previous) (diff)

#8 @costdev
2 years ago

I ran another test:

  1. Installed WP 5.8.
  2. Deleted Akismet.
  3. Edited Core with the changes in the PR.
  4. Upgraded to WP 5.9.3. - Akismet was not installed.

Note:
For anyone else testing, when upgrading to 6.0-RC2, Akismet will be installed. I've checked this with and without the changes in the PR and Akismet is installed regardless. Basically, test with actual releases, not RC (and likely not Beta either).

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


2 years ago

This ticket was mentioned in Slack in #core-auto-updates by costdev. View the logs.


2 years ago

#11 @SergeyBiryukov
2 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 54143:

Upgrade/Install: Remove _copy_dir() function as originally intended.

WordPress 3.2 introduced several enhancements to the copy_dir() function:

  • No more re-installing Akismet upon upgrade.
  • Respect custom WP_CONTENT_DIR for bundled plugins/theme installation.
  • Respect custom WP_CONTENT_DIR/WP_LANG_DIR for language files when upgrading.
  • Add an exclusion list to copy_dir() as well as WP_Filesystem_Base::wp_lang_dir().
  • Standardize WP_Filesystem path method returns.

However, the version of copy_dir() that runs during the upgrade process is the one from the older install, not the newer, which means that these enhancements would only be available after upgrading to WordPress 3.2 first, e.g. in a subsequent upgrade to WordPress 3.3.

In order to make these enhancements immediately available in WordPress 3.2, specifically to take advantage of skip lists and avoid re-installing Akismet if it was previously deleted, a temporary copy of the function was utilized, with the intention to remove it in WordPress 3.3 or a later release.

With further enhancements made to the Upgrade API to support partial and no-content builds, this temporary copy is no longer relevant and can be safely removed.

Follow-up to [17576], [17580], [17581], [18225].

Props afragen, costdev, dd32, peterwilsoncc, SergeyBiryukov.
Fixes #55712. See #17173.

SergeyBiryukov commented on PR #2703:


2 years ago
#12

Thanks for the PR! Merged in r54143.

Note: See TracTickets for help on using tickets.