Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#58995 closed defect (bug) (fixed)

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

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

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 3 years 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
3 years ago

I am taking up this issue.

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


3 years 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
3 years 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
3 years 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.


3 years ago

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


3 years ago

#6 @pbiron
3 years 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:


3 years 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
3 years ago

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

#9 @davidbaumwald
3 years ago

  • Owner set to davidbaumwald
  • Status new → reviewing

#10 @nicolefurlan
3 years 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.


3 years ago

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


3 years ago

#13 @nicolefurlan
3 years ago

  • Milestone 6.4 → 6.5

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

#14 @oglekler
3 years ago

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

#15 @davidbaumwald
3 years ago

Working on a merge for this before 6.5 Beta 1.

#16 @davidbaumwald
3 years ago

  • Resolution → fixed
  • Status reviewing → 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:


3 years 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.