Make WordPress Core

Opened 14 years ago

Closed 3 years ago

Last modified 2 years ago

#17173 closed defect (bug) (fixed)

Review the usage of _copy_dir() introduced in #14484

Reported by: dd32's profile dd32 Owned by: dd32's profile dd32
Milestone: 3.2 Priority: normal
Severity: normal Version:
Component: Upgrade/Install Keywords: 3.3-early has-patch
Focuses: Cc:

Description

Review the usage of _copy_dir() introduced in #14484;

Back story: Due to enhancements made to copy_dir() in 3.2 development which #14484 took advantage of, in order to bring immediate relief to users, a temporary function was utilised.

We should be able to remove that in 3.3, However, Removing it will harm those upgrading from pre-3.2 to 3.3, for the majority of users this will not be a problem (As long as they don't skip a version).

Change History (17)

#1 @dd32
14 years ago

  • Status changed from new to accepted

#2 @nacin
14 years ago

  • Milestone changed from Future Release to 3.2

Mark suggested we should just build and serve a special package for upgrades on dotorg and revert the changes. Makes sense considering we're also doing partials. Let's talk Wednesday.

#3 @westi
14 years ago

@dd32 @nacin - Do we need to do anything here for WordPress 3.2 ?

#4 @dd32
14 years ago

I'm not sure if Nacin has completed the work which would've mean't removing this from core before release. At this stage, I believe this is to stay for 3.2 due how close a release is..

#5 follow-up: @sterlo
14 years ago

Introducing code that would keep people using < 3.2 from upgrading to 3.3 could cause some serious issues.

"Minor version Y (x.Y.z | x > 0) MUST be incremented if new, backwards compatible functionality is introduced to the public API. It MAY be incremented if substantial new functionality or improvements are introduced within the private code. It MAY include patch level changes."

"Major version X (X.y.z | X > 0) MUST be incremented if any backwards incompatible changes are introduced to the public API. It MAY include minor and patch level changes."

http://semver.org/

Thoughts?

#6 in reply to: ↑ 5 @westi
14 years ago

Replying to sterlo:

Introducing code that would keep people using < 3.2 from upgrading to 3.3 could cause some serious issues.

"Minor version Y (x.Y.z | x > 0) MUST be incremented if new, backwards compatible functionality is introduced to the public API. It MAY be incremented if substantial new functionality or improvements are introduced within the private code. It MAY include patch level changes."

"Major version X (X.y.z | X > 0) MUST be incremented if any backwards incompatible changes are introduced to the public API. It MAY include minor and patch level changes."

http://semver.org/

Thoughts?

I'm not sure what this has to do with this ticket at all.

One of our design goals is backwards compatibility and we don't inflate version numbers.

#7 @dd32
14 years ago

Thoughts?

There is no huge "Backwards incompatible change" here, There is a bug, which has affected the last half a dozen releases which is fixed for 3.2 onwards, those upgrading who skip the 3.1-3.2 step might will trigger it again, however, it's not a harmful bug, it's one that people have lived with over time (akismet getting re-installed upon upgrade). Even then, That might be nulled by a API change that's in the works.

to make it clear: Those upgrading from 2.9 to 3.3 will be able to do it, this change will not prevent that, They will however, get akismet re-installed if they've removed it.

furthermore, That version numbering is not the version numbering WordPress utilises. (x.y) is the major version, (x.y.z) is the "patch" version(We don't have a "minor" release, the latter is generally called a minor release, or a point release), 2.8->2.9 holds the same weight as 2.9->3.0, which will be the same as 3.9->4.0.

#8 @aaroncampbell
14 years ago

Honestly I don't see why we can't just make the changes to the API, rely on them completely, and remove the function right now. This temporary fix seems kind of messy (not the code, just the process of adding it in and taking it back out).

I guess the main reason is so we can announce partial upgrades as a 3.2 feature? If that's the case it seems to me like partial upgrades are really only a "faster, lighter" feature if the download is smaller, right?

#9 @nacin
14 years ago

I spent a long time with a giant whiteboard with koopersmith and PeteMall at WordCamp Reno dealing with some of the roadblocks I've been hitting implementing a new API and build process.

While working on the new API, I had been under the impression that we could just wire in the new no-plugins-no-themes zip into the existing API version, and avoid our _copy_dir() hack for 3.2.

The issue is that our current API is inadequate. Here it is:

upgrade
http://wordpress.org/download/
http://wordpress.org/wordpress-3.1.3.zip
3.1.3
en_US
4.3
4.1.2

In particular, on update-core.php, there's a button to download the newest version. It presumably once linked to /download/, the second line returned. But it now links directly to the zip, which is where we'd need to shoehorn in our own custom zip. So you see the problem: The same URL is used for two different things. No shoehorning allowed.

So, here is what I am doing and proposing:

  • I have been working on a new build script on WP.org for content-less builds and also partial builds. This is quite complicated, as there's an entirely different build process for languages as well. The main one should be done soon, and I'm still looking into the languages complication. At worst, we flip a switch when that is done.
  • We need a new API. We were going to do this for partial builds anyway, but we need one now. So, I'll be posting a patch to switch over to /1.6/ hopefully as early as today, which will be a serialized response that can scale to support partial builds, and importantly will support content-less builds.
  • We need to leave _copy_dir() in for one or two release cycles. We can leave this in now for the immediate 3.1 to 3.2 (maybe 3.3) benefit. It isn't grandfathered code we need to keep forever -- it's okay if someone updating from 3.1 to 3.4 (or 3.3) gets their Akismet and Hello Dolly stomped on.
  • Partial builds require some changes to core, particularly due to a decision we made on md5'ing files. Otherwise, it's just the matter of choosing a zip to download. I have code from dd32 that I've been massaging into a patch. My goal is to have everything working by tomorrow.

So, for 3.2, here's what I'm potentially thinking:

  • Leave in _copy_dir().
  • Have a new API that returns whatever we need or will need. Just a matter of wiring it in.
  • No UI changes at this time.
  • No partial build support. However, we'll have the ability to tell core to instead use the partial build URL through a temporary constant or filter. Thus we can test this through the 3.2.x cycles, and include md5 checking in 3.3.

That said:

The new API, once fully functional, can be wired in at any time. Can probably do it today. But if partial builds are near and dear to anyone, then you're either out of luck or we need to scrap md5 implementation as a project requirement.

I think md5 is a nice-to-have, rather than a requirement. A button "Update to latest" can trigger the partial build, and "Full re-install" would trigger a full upgrade/reinstall. This would be easy to implement -- it requires the new API, which we're doing anyway, and a new button (if we can do a partial build).

#10 @nacin
14 years ago

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

_copy_dir() stays until further notice, as indicated in the above comment.

Everything else has been carried into #10611.

#11 follow-up: @afragen
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@dd32 is it time for this to now be removed?

https://wpdirectory.net/search/01G2TWQVBE04Z8BW7BG6KB321T only a single use in the wild and it's in a function_exists().

#12 in reply to: ↑ 11 @dd32
3 years ago

Replying to afragen:

dd32 is it time for this to now be removed?

I expect so, I'm not fully up-to-date on how core upgrades work at present, but a quick grep suggests to me that yes, might as well be removed now.

#13 @afragen
3 years ago

I can put together a PR. There are 2 lines that need to be changed in update-core.php

Last edited 3 years ago by afragen (previous) (diff)

This ticket was mentioned in PR #2702 on WordPress/wordpress-develop by afragen.


3 years ago
#14

  • Keywords has-patch added

As originally described, adding _copy_dir() was supposed to be a short term fix for updating 3.1 -> 3.2 installations. I think it's time and we can clean up some duplicate code. I believe the original issue and objections have been sorted out in the Updates API.

Trac ticket: #17173

#15 @afragen
3 years ago

Created new ticket #55712 as this milestone is closed.

#16 @afragen
3 years ago

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

#17 @SergeyBiryukov
2 years ago

In 54143:

Upgrade/Install: Remove _copy_dir() function as originally intended.

WordPress 3.2 introduced several enhancements to the copy_dir() function:

  • No more re-installing Akismet upon upgrade.
  • Respect custom WP_CONTENT_DIR for bundled plugins/theme installation.
  • Respect custom WP_CONTENT_DIR/WP_LANG_DIR for language files when upgrading.
  • Add an exclusion list to copy_dir() as well as WP_Filesystem_Base::wp_lang_dir().
  • Standardize WP_Filesystem path method returns.

However, the version of copy_dir() that runs during the upgrade process is the one from the older install, not the newer, which means that these enhancements would only be available after upgrading to WordPress 3.2 first, e.g. in a subsequent upgrade to WordPress 3.3.

In order to make these enhancements immediately available in WordPress 3.2, specifically to take advantage of skip lists and avoid re-installing Akismet if it was previously deleted, a temporary copy of the function was utilized, with the intention to remove it in WordPress 3.3 or a later release.

With further enhancements made to the Upgrade API to support partial and no-content builds, this temporary copy is no longer relevant and can be safely removed.

Follow-up to [17576], [17580], [17581], [18225].

Props afragen, costdev, dd32, peterwilsoncc, SergeyBiryukov.
Fixes #55712. See #17173.

Note: See TracTickets for help on using tickets.