WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 7 years ago

#17816 closed defect (bug) (fixed)

WordPress in dev environments should update /wp-content/* again

Reported by: ocean90 Owned by:
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.2
Component: Upgrade/Install Keywords:
Focuses: Cc:
PR Number:

Description

It seems like if you are have a dev version and update from 3.2 Beta to 3.2 RC /wp-content/* isn't touched, means in this case Twenty Eleven won't be updated.

This results in confusion, see for example #17603.
Also the comment by dd32:

This was indeed fixed, you however most likely have an out of date TwentyEleven installation. Due to a bug in the upgrade code, it looks like Twenty Eleven didn't get updated when you updated Core.

Which bug?

In my opinion if you are running a dev environment wp-content should be updated again.

Attachments (3)

17816.patch (626 bytes) - added by ocean90 8 years ago.
17816.diff (3.1 KB) - added by dd32 8 years ago.
17816.2.diff (678 bytes) - added by nacin 8 years ago.

Download all attachments as: .zip

Change History (24)

#1 @ocean90
8 years ago

Related: #14484

#2 @nacin
8 years ago

Not entirely sure how this should work. Always update themes, that makes sense. But what about Twenty Ten? (Is it staying in the download package? Seems like it.) Or future Hello Dolly changes? We want to encourage people to update via the Beta Tester plugin, so I'd rather keep it as close as possible to how it works normally.

That said, the theme should always be updated. How can we check if we're updating to a development version from here? Are we able to check the transient for response == 'development'? If so, that seems better than parsing the existing and incoming wp_versions.

We do a $type check in the update code. Perhaps we can make this only apply to themes, and only the final theme in the array. (If we end up introducing two themes at once or something, we can always just change this code.) Or maybe this simply becomes another thing to bump? We already bump versions in wp-admin/includes/update-core.php. We could also do $force_reinstall = array( 'twentyeleven' ) and add/remove as appropriate. (And only force this for development versions.)

Regardless of what we do, let's make it filterable that way the Beta Tester plugin can add a checkbox or something, if westi wants.

@ocean90
8 years ago

#3 @ocean90
8 years ago

  • Keywords has-patch added

17816.patch
The patch adds the option to filter the skip list. I think this is the easiest way.

For example if only the plugins dir should be skipped and themes should be updated:

function my_update_core_skip_list( $list ) {
	unset($list);
	$list[] = 'wp-content/plugins';
	return $list;
}
add_filter( 'update_core_skip_list', 'my_update_core_skip_list' );

#4 @dd32
8 years ago

This results in confusion, see for example #17603. Also the comment by dd32:

This was indeed fixed, you however most likely have an out of date TwentyEleven installation. Due to a bug in the upgrade code, it looks like Twenty Eleven didn't get updated when you updated Core.

Which bug?

A bug I found via that ticket that I havn't had the chance to trac yet.

Currently, as it turns out, anyone who has been beta testing and using the builtin upgrader, hasn't been receiving updates for TwentyEleven, TE is installed on the first update, however further updates to nightlies, etc, has been skipping TE; This has been caused by a misplaced exists() check, as shown in #17603 where the user hadn't received the RTL changes in a nightly update.

Me and nacin have touched on this issue in private conversation very lightly, updates to future core-bundled files really need to go throgh their respective upgraders, relying upon the core update to update the other items (which will usually have some kind of their own update schedule) is what has been resulting in the downgrade dance.

The changes to ignore wp-content have 2 different purposes

  1. To copy files to the correct folder locations when WP_LANG_DIR, and WP_PLUGIN_DIR have been defined
  2. To skip overwriting existing versions of bundled items when upgrading.
    1. The bug that exists here, is it's not overwriting in nightly releases too.

#5 @dd32
8 years ago

  • Milestone changed from Awaiting Review to 3.2

Just bumping this into 3.2 (where I thought it was already).

Nacin, Can you check this over and see what you think?

@dd32
8 years ago

#6 @nacin
8 years ago

Don't see why we're removing the exists checks. Otherwise makes sense. You're right though, RC2 to final won't work right. Maybe we could detect that we're on a dev and updating to stable of the same version? If necessary the API could return extra information.

#7 @nacin
8 years ago

Yeah, if necessary, the API could use reported and new versions to return an array of "yes copy over these files anyway". I'd have no problem doing that and it'd pull some of the logic out of core.

If we can nail down a grid of reported and upgrade-to versions and what should happen, I can make it happen.

#8 @nacin
8 years ago

I think we need to have an IRC conversation with ryan, westi, dd32 to resolve this. Let's aim for early this week.

#9 @dd32
8 years ago

I think we need to have an IRC conversation

Quite agree.

Don't see why we're removing the exists checks.

Quite simply, They shouldn't've been there in the first place, an error on my part.. That being said, by having them there, it prevents overwriting of the files if they've been copied into place already, so if someone installs TwentyEleven on 3.1, and upgrades to 3.2, they won't get the updated copy of the theme. etc..

It might be better to not remove them, and simple change it to

if ( !$development_upgrade && $wp_filesystem->exists($dest . $filename) ) 
    continue; 

If we cant rely upon the development response, i'd call a development upgrade to be anything where, the previous, or new version, contains a dash in the version string.. not the most reliable, but certainly currently compatible.. (Need to check if debian and friends hammer their own suffixes in though)

#10 @azaozz
8 years ago

Thinking it's too late for this to affect 3.2 in any significant way (RC2 is currently out). Perhaps leave it for 3.3.

#11 @nacin
8 years ago

The one issue is anyone testing the betas won't have a new Twenty Eleven until we put it in the repo, which based on Twenty Ten's timeline we won't do until 3.2.1.

Can we just hardcode in an exception to continue to copy over Twenty Eleven until the new $wp_version >= 3.2? That would satisfy the remaining days of testing and we can deal with it in 3.2.1/3.3.

#12 @dd32
8 years ago

The alternative there, would be to just leave as-is for now, Bump TwentyEleven to 1.1 just before release, and add TwentyEleven 1.1 to the repo as soon as we release, that'll allow beta testers to update the theme on upgrade.

Perhaps in future, we should not label things as 1.0 until it's actually at 1.0 :)

#13 @nacin
8 years ago

Shipping with 1.1 will be confusing. I'm fine with not bumping until we're ready, in the future. We had done that (for the most part) with Twenty Ten. I'd rather not rush things into the directory. The process for Twenty Ten worked out well for our own purposes.

How bad would the patch I suggested be? Seems like just one conditional; I can tackle it tomorrow if necessary.

#14 @dd32
8 years ago

Simple, release with 20.11 as the version then ;)

Honestly, off the top of my head, I don't think the patch would be too complex, just an extra conditional in the return-if-exists branch.

#15 @mattyrob
8 years ago

Just a thought for the future and such issues, maybe while themes like TwentyEleven are in development it would be wise to have a version number like 0.5 or 0.9 and bump this to 1.0 on release with the WordPress core. Wouldn't that work? (In future I mean - too late now obviously for TwentyEleven)

#16 @dd32
8 years ago

Just a thought for the future and such issues,

Definitely, Which is why I mentioned that above :)

Perhaps in future, we should not label things as 1.0 until it's actually at 1.0 :)

Ultimately, We're going to have to add the theme to the repo in order to get updates to theme, as the 3.2.1 upgrade isn't going to upgrade TwentyEleven as well at this point in time.

@nacin
8 years ago

#17 @nacin
8 years ago

Here's a patch that could go in now and be pulled out right after 3.2 ships.

Alternative is to bump to 1.1 now and put it in the directory.

#18 @nacin
8 years ago

  • Keywords punt added; dev-feedback 2nd-opinion has-patch removed

Per IRC conversation, seems best to just ship 1.1 and put it in the directory immediately. Beta testers will then need to update, which seems fine to ask them of.

#19 @nacin
8 years ago

  • Keywords punt removed
  • Milestone changed from 3.2 to Future Release

Mentioned the bump in #17759. Punting.

#20 @ocean90
8 years ago

It happens again, see #19409.

#21 @dd32
7 years ago

  • Milestone changed from Future Release to 3.5
  • Resolution set to fixed
  • Status changed from new to closed

Fixed by [22226]

Note: See TracTickets for help on using tickets.