Make WordPress Core

Opened 8 weeks ago

Last modified 12 days ago

#58995 reviewing defect (bug)

Remove individual files from `$_old_files` array when its parent directory is already included

Reported by: davidbaumwald's profile davidbaumwald Owned by: davidbaumwald's profile davidbaumwald
Milestone: 6.4 Priority: normal
Severity: normal Version: 6.3
Component: Upgrade/Install Keywords: good-first-bug has-patch
Focuses: Cc:

Description

Chatting with @SergeyBiryukov, it seems that $wp_filesystem->delete( $old_file, true ) is called on entries in the array and the second parameter, $recursive, is set to true. So, individual files aren't necessary when the parent directory is included.

Some precedent exists higher up for older version where individual files were excluded, such as wp-includes/js/tinymce/plugins/wpembed.

Props to @SergeyBiryukov for the discussion.

Attachments (1)

update-core-ticket-58995.diff (12.7 KB) - added by mhshohel 7 weeks ago.
Attaching the differential file separately for visual reference, allowing anyone to easily observe the highlighted differences.

Download all attachments as: .zip

Change History (10)

#1 @abhi3315
8 weeks ago

I am taking up this issue.

This ticket was mentioned in PR #4982 on WordPress/wordpress-develop by shohel.


7 weeks ago
#2

  • Keywords has-patch added; needs-patch removed

The implemented update ensures that when processing the $_old_files array, any individual files referenced within the array are excluded from deletion if their corresponding parent directories are already included.

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

#3 @mhshohel
7 weeks ago

@davidbaumwald @abhi3315

I've eliminated all the individual files that are already encompassed by their parent directory within the $_old_files array.

@abhi3315, my apologies for addressing this matter, as it appears to be consuming more time than anticipated.

@mhshohel
7 weeks ago

Attaching the differential file separately for visual reference, allowing anyone to easily observe the highlighted differences.

This ticket was mentioned in Slack in #core-upgrade-install by pbiron. View the logs.


5 weeks ago

This ticket was mentioned in Slack in #core-upgrade-install by costdev. View the logs.


4 weeks ago

#6 @pbiron
4 weeks ago

@davidbaumwald isn't the $_old_files array generated by tooling as part of the release process? If that's true, shouldn't that tooling simply be updated to not include a file in the array if a dir in it's path has already been included in the array?

@mhshohel commented on PR #4982:


4 weeks ago
#7

@pbiron

Hey Paul, thank you for noticing it, I've removed these two files as their parent wp-includes/js/codepress directory already exists

#8 @pbiron
4 weeks ago

@davidbaumwald the PR LGTM, but it wouldn't hurt to have another set of eyes on it.

#9 @davidbaumwald
12 days ago

  • Owner set to davidbaumwald
  • Status changed from new to reviewing
Note: See TracTickets for help on using tickets.