WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 3 years ago

#31420 closed defect (bug) (fixed)

license.txt and readme.html being unwritable shouldn't block a Background Update

Reported by: dd32 Owned by: dd32
Milestone: 4.7 Priority: normal
Severity: normal Version: 3.7
Component: Upgrade/Install Keywords: has-patch needs-testing
Focuses: Cc:
PR Number:

Description

As one of the preflight checks we perform before performing a background update, we check that every file that changed in the update is writable.
As it turns out, readme.html and license.txt are sometimes unwritable while all other files are writable. For an Installed version of WordPress, neither of these files contains information which strictly needs keeping up to date (license.txt changes should only ever be date changes, and readme.html changes should only be a version number bump).

Proposal - If only license.txt & readme.html are unwritable, proceed with the update and skip the files.
Complications - Some languages include a translated license and readme, such as liesmich.html(de_DE) and licenza.html(it_IT)
Counter proposal - Don't require *.txt or *.html to be writable if they exist in the root directory (As we're not adding any other root files, and these are the only few that exist at present).

The code to check these files are writable exists within wp-admin/includes/update-core.php so we also have the ability to backport this fix to a minor release.

Attachments (1)

31420.diff (640 bytes) - added by polevaultweb 4 years ago.

Download all attachments as: .zip

Change History (14)

#1 @swissspidy
4 years ago

  • Keywords needs-patch added

@polevaultweb
4 years ago

#2 @polevaultweb
4 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

Patch added, feedback welcome :)

#3 @dd32
4 years ago

  • Milestone changed from Future Release to 4.6
  • Owner set to dd32
  • Status changed from new to accepted

@polevaultweb Interesting approach there :)

I think we should probably go for a hard-coded list of files instead myself, as it'll make the code a little more readable and explicit.

#4 follow-up: @polevaultweb
4 years ago

@dd32 thanks - i think! :)

I was going for the wildcard filename / extension check as you had mentioned the complication of translated filenames. Wouldn't we need to update the list each time a new language for WP was added?

#5 in reply to: ↑ 4 @dd32
4 years ago

Replying to polevaultweb:

@dd32 thanks - i think! :)

I was going for the wildcard filename / extension check as you had mentioned the complication of translated filenames. Wouldn't we need to update the list each time a new language for WP was added?

I completely forgot about that :) Looks good to me then, I'll look at getting this in soon
Thanks!

#6 @polevaultweb
3 years ago

@dd32 did you get chance to look at this :)

#7 @Presskopp
3 years ago

In case the files are not writable, and we skip them to keep the update running, it would lead to

1) empty or stupid readme.html and/or license.txt (or similar in different language)
(because user write-protected them for security-reasons or whatever)

OR

2) outdated readme.html and/or license.txt (they are genuine, but old)

So why not drop the version number then, see #23394 ?

#8 @ocean90
3 years ago

@polevaultweb I took a look and the current patch seems incomplete because of this check: https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/update-core.php?marks=964#L955. It would compare the checksum of an old file with the checksum of the new file.

#9 @ocean90
3 years ago

  • Keywords needs-refresh added
  • Priority changed from normal to low

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


3 years ago

#11 @ocean90
3 years ago

  • Keywords early added
  • Milestone changed from 4.6 to Future Release
  • Priority changed from low to normal

Punting due to lack of traction.

#12 @dd32
3 years ago

  • Keywords needs-refresh early removed
  • Milestone changed from Future Release to 4.7

#13 @dd32
3 years ago

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

In 38898:

Upgrade: Don't fail a core update just because readme.html or license.txt couldn't be modified.

A number of locked down installs remove readme.html or make it inaccessible which would result in an update failure.

Props polevaultweb for the initial patch.
Fixes #31420.

Note: See TracTickets for help on using tickets.