#55712 closed enhancement (fixed)
Remove _copy_dir() as originally intended
Reported by: | afragen | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 6.1 | Priority: | normal |
Severity: | normal | Version: | 3.2 |
Component: | Upgrade/Install | Keywords: | has-patch commit |
Focuses: | Cc: |
Change History (12)
This ticket was mentioned in PR #2703 on WordPress/wordpress-develop by afragen.
2 years ago
#1
#3
@
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.
#5
@
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
@
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:
- A
require/require_once/include/include_once
statement forwp-admin/includes/update-core.php
. - A reference just to
wp-admin/includes/update-core.php
.
After reviewing all 53 results:
- These are used either for calling
update_core()
, or making use of the$_old_files
array. - 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
orinclude
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 atupdate_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.
#7
in reply to:
↑ description
@
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 curent API handles this as expected.
#8
@
2 years ago
I ran another test:
- Installed WP 5.8.
- Deleted Akismet.
- Edited Core with the changes in the PR.
- 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
@
2 years ago
- Owner set to SergeyBiryukov
- Resolution set to fixed
- Status changed from new to closed
In 54143:
SergeyBiryukov commented on PR #2703:
2 years ago
#12
Thanks for the PR! Merged in r54143.
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