Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Here I'd like to test out some theories that using a quasi-patch zip file will work update things in place.
This currently depends on a new test type in #3031379: Add a new test type to do real update testing.
The only available update paths at this point (9/16/2019) are the ones listed in #3061248: Package quasi-patch for automatic updates. This will probably change to encompass more projects sometime soon.
Proposed resolution
Write some code
Remaining tasks
Do it.
Comment | File | Size | Author |
---|---|---|---|
#57 | 3055872-57.patch | 35.47 KB | heddn |
#57 | interdiff_56-57.txt | 510 bytes | heddn |
Comments
Comment #2
heddnHere's an initial PoC of what an in-place update would look like. This is simple at this point and manually tested. What do we think? It allows updating from 8.7.0 => 8.7.1. Sorta. The majority of that release is in the vendor folder. Which I didn't bundle. I only bundled the parts that are directly part of core for this PoC. But it demonstrates we'll need a system that does update the vendor folder. Not just the core folder.
Comment #3
heddnOK, here's a more complete picture of how things might work. Still need to incorporate the deletions.
Comment #4
heddnphpcs fixes only.
Comment #5
catchNot done a full review, but in general the 'get a zip, extract, copy the individual files over' seems quite vindicated by the patch, really doesn't look too bad so far.
Comment #6
heddnre #5: that feedback is really helpful. I had a similar reaction after looking at everything. There's some edge cases. We still need to account for deletions and rollbacks. But over-all, things are in a pretty good state.
Testing this whole thing is going to be complicated. I'm hopeful that #3031379: Add a new test type to do real update testing lands sometime soon to make this more possible.
Comment #7
catchFor deletions, does this need to use a manifest?
Comment #8
heddnre #7: Yes, I think so. @mbaynton commented that's how he's doing it today and I was going to incorporate that feature when I pick this up again.
Comment #9
heddnThis is manually tested and seems to work for the whole thing. Including deletions. Lots of code review and there might be edge cases that are unaccounted. But this shows that with a fairly small amount of code, in place updates can occur.
Next to work on tests.
Sorry there's no interdiff. It needed a re-roll and I didn't post that as a patch first.
Comment #10
heddnSuper basic test. Deletions happen last in the update process. So if the file is gone, we assume that the update process got that far. For a PoC, it is enough.
Comment #11
heddnGood green. I'd like to keep this focused on the service and its API (interactions) with an FTP site that hosts the files.zip files. In follow-ups, we'll wire the service into a (manual) button and (automated) cron to apply the updates.
Comment #12
catchOnly nitpicks, general approach looks good.
With the manifest do we definitely want this only for deletions, or for the included files too would be my only proper question.
I think this should be 'Constructs an InPlaceUpdate'?
Why sink for the variable name?
The comment says backup but the method says process.
Should this have a watchdog_exception()?
Same here.
Backup before an update?
Should this just assign to $backup first then $this->backup after the realpath call?
Comment #13
heddnre #12
0.5 (manifest): I had a similar thought at one point and eventually rejected it. Too troublesome to parse the manifest file for deletions vs creations, vs updates. Much easier to just delete all files listed in the file. Plus I'm not sure what the benefit (except completeness) for adding all the files.
1: fixed comment
2: renamed variable
3: fixed comment
4-5: the filesystem service already logs it, I don't think there is a need to double log.
6: fixed comment
7: fixed variable assignment
I also renamed constant value to
DELETION_MANIFEST.txt
Comment #15
heddnComment #16
heddnComment #17
catchThis is looking really solid to me. Is there an issue open to figure out actually creating the zip releases?
Comment #18
heddnAdded #3061248: Package quasi-patch for automatic updates just now.
Comment #19
heddnMinor re-roll
Comment #21
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThere have been some concerns expressed that the work here is not compatible with what's happening in the Composer in Core initiative, so I wanted to drill down on how the zip files are being generated.
First, a couple of definitions.
The Composer in Core initiative is going to deliver "Composer-ready" tarball downloads. That means that if you download Drupal and untar it, you can immediately cd into your directory and run
composer update
orcomposer require
, and the right thing will happen."Compsoer-ready" does not mean mix-and-match, though; once you start using Composer on an individual Drupal site, you cannot go back to
drush pm-update
and friends.Our goal, then, is that if #autoupdates does not support Composer use as an update method initially that it at least maintain the "Composer ready" nature of the tarball downloads.
If the result of applying a quasi-patch zip file to a site always produces the same result as un-taring the equivalent tarball download on top of the same site (i.e. it is not missing any files except files that did not change), then the "Composer-ready" aspect of the site should be maintained.
If that's the case, I think that's sufficient for now. Of course, autoupdates will have to use Composer to update for sites that have used Composer to change their vendor directory, but I think it's okay if that is follow-on work, after the quasi-patch technique, if that's the desired development order here.
Comment #22
heddnFrom the slack conversation during the regular scheduled autoupdates meeting today:
drush pm-update
in Drupal 8.8.0). Yeah!!! No additional modifications to anything besides what comes out of the gate from the conversion to composer. This should work in all cases, just like the previous tarball or drush install scenarios.composer.lock
file and modified vendor folder. This should also mostly just work, unless you’ve updated to a later version of symfony or twig or something than what d.o. says you should have. If the site has updated something from vendor, then we wouldn’t want to do harm and replace it with something potentially older or contrary to what is already installed. They don’t match the hash from d.o and we’d fail to update you. Because we’d be potentially causing more harm than good by overwriting some hacked or patches version of those files.Comment #23
heddnThe failures in #19 are due to us finally reaching a point where we need #3031379: Add a new test type to do real update testing
Comment #24
heddnOK, this builds off #3031379: Add a new test type to do real update testing. It passes locally, let's see if we got all the right things configured for testbot running.
Comment #25
heddnDoh,
--binary
Comment #26
heddnFixing phpcs
Comment #28
heddnComment #29
heddnComment #30
heddnComment #31
heddnThis is now using the zip file format that @drumm plans to use on d.o. If this passes green (it did locally) it is ready for reviews.
I plan to do the signature validation in a follow-up.
Comment #33
heddnComment #34
heddnHere's the start of adding test coverage for contrib projects. It doesn't pass locally, but its some more progress.
Comment #36
heddnThis ran green on local now for contrib testing.
Comment #37
heddnUff,
--binary
Comment #38
heddnAdd that newline for code standards. But otherwise, this is green and ready for reviews.
Comment #39
heddnComment #40
heddnWe already depend on d.o assets for composer install and other things, let's go the full way and do that for the update assets too.
Comment #41
eiriksmWow this looks awesome!
There are a couple of things I had to figure out here, for testing. It currently uses a drupal.org link to find the quasi patch zips, and just trying to update a module I had locally (admin_toolbar) did not work. I assume because only token and bootstrap are available yet? Maybe we should update the IS with that?
Also, I had to look for a while to find out where I would get the QuickStartTestBase from, but figured out it was from this issue: #3031379: Add a new test type to do real update testing
So, code looks pretty solid. Some minor things and nits mostly from me :)
This is currently not being used for anything. Is it intended to be used later?
This can theoretically return FALSE or throw an exception. Seems we should at least check for a FALSE value here?
$version is not defined. I think you meant $to_version ? :)
getRealPath can return a bool, which does not go well together with substr. Maybe check for that?
The variable result is never used for anything
Another one with a potential bool|string
Another one with a potential bool|string
bool|string again
Comment #42
heddnComment #43
heddnThis fixes some of the feedback from #41. I'm not sure how
getRealPath
could return a non-string though, so I didn't fix that.This also adds more test coverage.
Comment #44
heddn41.2 seems like it would throw an exception if the URL is malformed, not an empty string. I don't want to catch that but rather things should be loud and noisy in that case:
41.4:
rtrim
always returns a string. Ifsubstr
returns false, it won't match 'vendor'. It seems like all cases, we get a string, no?Comment #45
heddnComment #46
eiriksmSorry I was not more explicit. But I was talking about $file->getRealPath():
https://www.php.net/manual/en/splfileinfo.getrealpath.php
Comment #47
eiriksmAlthough to be fair, that would be pretty unlikely, since we filter for $file->isFile() not long before. As I said. Very nit, sorry :)
Otherwise I feel my feedback was very well addressed, nice update in the IS as well.
Comment #48
heddnComment #49
heddnComment #50
heddnComment #51
eiriksmThanks, looking great!
Comment #52
heddnComment #53
heddnThe actual code under test still hasn't changed since #51. All that is changing is updates for chasing core HEAD and its new test type in #3031379: Add a new test type to do real update testing.
If this still is green after a test run, I'm going to land this thing today.
Comment #54
heddnComment #56
heddnComment #57
heddnComment #59
heddnThanks for all the reviews.
Comment #60
hestenetWoohoo!