Make WordPress Core

Opened 8 months ago

Closed 2 months ago

Last modified 2 months ago

#58995 closed defect (bug) (fixed)

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.5 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 8 months ago.
Attaching the differential file separately for visual reference, allowing anyone to easily observe the highlighted differences.

Download all attachments as: .zip

Change History (18)

#1 @abhi3315
8 months ago

I am taking up this issue.

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


8 months 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
8 months 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
8 months 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.


8 months ago

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


8 months ago

#6 @pbiron
8 months 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:


8 months 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
8 months ago

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

#9 @davidbaumwald
7 months ago

  • Owner set to davidbaumwald
  • Status changed from new to reviewing

#10 @nicolefurlan
6 months ago

@davidbaumwald I'm wondering if we'll be able to include this fix in 6.4 RC1, which is just ~2 weeks away. It seems that we need some testing instructions and then some testers to check if the patch works. What do you think?

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


6 months ago

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


6 months ago

#13 @nicolefurlan
6 months ago

  • Milestone changed from 6.4 to 6.5

Moving this to 6.5 per discussion in today's bug scrub.

#14 @oglekler
4 months ago

@davidbaumwald I checked the PR, and it looks like everything is accurate. Can you, please, progress with the review? Thank you!

#15 @davidbaumwald
2 months ago

Working on a merge for this before 6.5 Beta 1.

#16 @davidbaumwald
2 months ago

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

In 57598:

Upgrade/Install: Remove unnecessary individual subfiles from $_old_files array.

On upgrade, the $_old_files array is used to cleanup any files that exist in a previous version of core but are no longer present in the current version. Sometimes, an entire directory should be removed. In the past, when a parent directory was included in the array, subfiles were also included for good measure.

However, the code that removes the old files uses $wp_filesystem->delete() with the $recursive parameter set to true. With this setup, individual subfiles are not required to be individually listed when their parent directory is already included in the $_old_files array.

This commit removes all individual subfiles from the $_old_files array when their parent directory is already included.

Props SergeyBiryukov, mhshohel, pbiron, oglekler.
Fixes #58995.

@davidbaumwald commented on PR #4982:


2 months ago
#17

@shohel Thanks for the PR! Merged into core in https://core.trac.wordpress.org/changeset/57598.

Note: See TracTickets for help on using tickets.