New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DX] Add core updates via Backdrop UI. #3105

Closed
serundeputy opened this Issue May 13, 2018 · 60 comments

Comments

Projects
None yet
10 participants
@serundeputy
Member

serundeputy commented May 13, 2018

Describe your issue or idea

In #2018 we are interested in providing the ability for automatic updates to Backdrop core. As an initial step towards that goal this issue adds the ability to manually update Backdrop core via UI at /admin/modules/update

Actual behavior (if reporting a bug)

Currently at /admin/modules/update core updates are relegated to there own Manual section that prompts the user to download Backdrop and then move the files around themselves via server tools or cli.

Expected behavior (if reporting a bug)

Allow updating of core by selecting the core update from /admin/modules/update from the list the same way you can update modules.

Relevant version/system information (if applicable)

1.x


PR: backdrop/backdrop#2186 this one was corrupted w/ .swp file
PR: backdrop/backdrop#2201
PR: backdrop/backdrop#2289

@serundeputy

This comment has been minimized.

Member

serundeputy commented May 13, 2018

This one can not be tested via sandbox; since sandboxes are downloading 1.x backdrop (which does not need an update)

To test (I"m using lando so I can have drush and things I need):

  • Initiate a backdrop site
lando init

select backdrop

  • Start backdrop app (so we have access to drush):
lando start
  • download an old backdrop
lando drush dlb backdrop --select

Select an eld backdrop I chose 1.9.3 which at the time of this writing was option 2

  • install backdrop
lando drush si --db-url=mysql://backdrop:backdrop@database/backdrop

Make note of the username and password (you can install via UI as well, this is just a little faster if say you want to test this 11 times).

wget https://patch-diff.githubusercontent.com/raw/backdrop/backdrop/pull/2186.patch
  • apply
patch -p1 < 2186.patch

you should see output similar to:

geoff@yep backdrop exit code: [0] $ patch -p1 < 2186.patch
patching file core/modules/system/system.updater.inc
patching file core/includes/updater.inc
patching file core/modules/installer/installer.authorize.inc
patching file core/modules/installer/installer.manager.inc
patching file core/modules/installer/installer.module
patching file core/modules/system/system.module
Hunk #1 succeeded at 2113 (offset -48 lines).
Hunk #2 succeeded at 3829 (offset -48 lines).
patching file core/modules/system/system.updater.inc
  • clear cache so Backdrop can pick up on the new class CoreUpdater
lando drush cc all
  • visit: /admin/modules/update; you should see:

screen shot 2018-05-13 at 1 52 47 pm

  • select the Backdrop update checkbox and proceed as normal.
@findlabnet

This comment has been minimized.

findlabnet commented May 13, 2018

Sorry if I lost something, but for doing such all document root should be writable for web-server?

@herbdool

This comment has been minimized.

herbdool commented May 13, 2018

Who knew it was this easy?!

It worked smoothly for me. I tested with Lando using @serundeputy's steps -- quite helpful!

A few things:

  • This is pretty awesome.
  • swp file shouldn't be in the PR I'm guessing
  • We should change the links on the status page to point to the Update module tab instead of where it points now. Currently someone would click on the red dot at the top:

screenshot from 2018-05-13 19-52-05

Then when the click Available Updates it brings them to:

screenshot from 2018-05-13 19-45-30

which sadly makes it seem like all you can do is download the release and manually upgrade.

  • Is it just installing the /core folder or also index.php, .htaccess and so on? I couldn't tell from the code. If the latter should we put a warning that any customizations to .htaccess or the like will be lost?

Update: I think I found the part in the code where it's just copying over the core directory. So if there are files outside of /core that need updating should it be flagged as can't update via UI? Or should there be a warning that people will need to do some manual steps after core is updated?

@herbdool

This comment has been minimized.

herbdool commented May 14, 2018

Maybe we need to add ability for people to disable these updates in settings.php similar to how Wordpress does it https://codex.wordpress.org/Configuring_Automatic_Background_Updates. Or maybe not as part of this step since it's not automated and there are permissions already for updates.

@serundeputy serundeputy added this to the 1.11.0 milestone May 14, 2018

@laryn

This comment has been minimized.

Contributor

laryn commented May 14, 2018

Haven't tested yet but will do so as soon as I can (looks awesome!). The screenshot brings to mind this issue: #3039

(e.g. core updates show up under "Update Modules"? Maybe it's time just to have a single "Update" page that lists core, modules, layouts, themes...)

@olafgrabienski

This comment has been minimized.

olafgrabienski commented May 14, 2018

Wow, I've successfully updated a Backdrop 1.9.3 remote test site (on shared hosting) via the UI without any problem. Very promising!

(Will try to do the same with another test site soon, just to see if it works with different hosting services.)

@serundeputy

This comment has been minimized.

Member

serundeputy commented May 14, 2018

@herbdool

swp file shouldn't be in the PR I'm guessing

  • removed; thaks

We should change the links on the status page to point to the Update module tab instead of where it points now. Currently someone would click on the red dot at the top:

  • updated PR with link from /admin/reports/updates to /admin/modules/update

screen shot 2018-05-14 at 5 22 17 pm

Is it just installing the /core folder or also index.php, .htaccess and so on? I couldn't tell from the code. If the latter should we put a warning that any customizations to .htaccess or the like will be lost?

  • yes; just updating the core directory; this is how drush works too; and is the behaviour that I personally want; I don't want the update to hit my robots.txt, .htaccess or .gitignore or settings.php on a live site, but we should think about what to do if these files should have something relevant (security wise?), seems unlikely, but if there are ideas i'm glad to hear them

@olafgrabienski, @laryn, @herbdool thanks for testing; if you test further download the latest patch please:

wget https://github.com/backdrop/backdrop/pull/2186.patch
@klonos

This comment has been minimized.

Member

klonos commented May 14, 2018

cc'ing @mbaynton

@quicksketch

This comment has been minimized.

Member

quicksketch commented May 15, 2018

but we should think about what to do if these files should have something relevant (security wise?), seems unlikely, but if there are ideas i'm glad to hear them

For .htaccess files (or other potential updates to root files), we typically parse them on status checks to make sure they have the needed information in them. Same thing for settings.php. If something is amiss in those core files, we have been (and should continue to) flag problems in system_requirements(). I also agree we not should be overwriting those files, and attempting to merge/update them is a potentially messy business.

Edit: Corrected to say we should not be overwriting.

@serundeputy

This comment has been minimized.

Member

serundeputy commented May 15, 2018

@quicksketch

I also agree we should be overwriting those files

did you mean you agree we should not be overwriting ... ?

@olafgrabienski

This comment has been minimized.

olafgrabienski commented May 15, 2018

@serundeputy I've tried to get the latest patch release for another test, using wget https://github.com/backdrop/backdrop/pull/2186.patch as recommended by you. I guess that didn't work for me, at least there was no link to admin/modules/updates on the Available updates page. The auto-update worked however, just like my test yesterday.

Maybe I didn't answer the "Assume" question during patching correctly? (The first time I answered with n and the patch hadn't been applied, so I started from scratch, answering this time with y.)

patch -p1 < 2186.patch
patching file core/modules/system/system.updater.inc
patching file core/includes/updater.inc
File core/modules/installer/.installer.authorize.inc.swp: git binary diffs are not supported.
patching file core/modules/installer/installer.authorize.inc
patching file core/modules/installer/installer.manager.inc
patching file core/modules/installer/installer.module
patching file core/modules/system/system.module
Hunk #1 succeeded at 2113 (offset -48 lines).
Hunk #2 succeeded at 3829 (offset -48 lines).
patching file core/modules/system/system.updater.inc
The next patch would delete the file core/modules/installer/.installer.authorize.inc.swp,
which does not exist!  Assume -R? [n] y
File core/modules/installer/.installer.authorize.inc.swp: git binary diffs are not supported.
patching file core/modules/update/update.theme.inc
@olafgrabienski

This comment has been minimized.

olafgrabienski commented May 15, 2018

Another thing: After updating, on core/authorize.php I read:

backdrop
Installed backdrop successfully

and

Next steps
Your projects have been downloaded and updated.
Run database updates

I guess, "Your projects have been downloaded and updated." should move above the "Next steps" section.

@serundeputy

This comment has been minimized.

Member

serundeputy commented May 15, 2018

@olafgrabienski since you already have a patch applied the new patch did not apply.

You'll either want to start with a fresh install of Backdrop < 1.9.5 and apply the patch

or

Figure out the answers to apply the new patch on top of the old patch.

I've just been blowing away the install and getting a new pre 1.9.5 backdrop release

you can do that with:

drush dlb backdrop --select

thanks for testing!

@herbdool

This comment has been minimized.

herbdool commented May 15, 2018

@serundeputy I don't see any difference on the status report after trying your latest patch.

I think instead of linking the title "Security update required" we should make it an action: Update Backdrop and then we either reword "Download" to "Alternative: download" or something, or we get rid of it altogether. For those who are manually updating they already know how to get the file, everyone else will have a clear option to upgrade via the UI.

@serundeputy

This comment has been minimized.

Member

serundeputy commented May 15, 2018

thanks @herbdool

I don't see any difference on the status report after trying your latest patch.

  • the difference is that the Security update required text is now a link to /admin/modules/update

I think instead of linking the title "Security update required" we should make it an action: Update Backdrop and then we either reword "Download" to "Alternative: download" or something, or we get rid of it altogether.

  • I just did it this way because that is what happens for modules that appear on the updates page. That doesn't mean we have to keep it that way, but that is the way it is now. So, before changing that too much I'd like to have a consensus on the change. Otherwise the dev (me in this case) ends up writing a lot of code that chase disparate ideas.

How we come to consensus has always been a mystery to me ;) ; let's see if others chime in.

@serundeputy

This comment has been minimized.

Member

serundeputy commented May 15, 2018

oh; i see you are saying no change on status report; yep i did not change that as it already links to admin/modules/update and the other link goes to the updates list. Is this not the desired behaviour?

@herbdool

This comment has been minimized.

herbdool commented May 15, 2018

I don't see either change.

This is what I see at admin/reports/status:

screenshot from 2018-05-15 11-22-21

has both links going to /admin/reports/updates and that looks like:

screenshot from 2018-05-15 11-22-40

which still looks like the old format.

@serundeputy

This comment has been minimized.

Member

serundeputy commented May 15, 2018

@herbdool I'm guessing either the patch isn't applied (maybe it failed) or cache. It is working for me:

bcms-updates

@herbdool

This comment has been minimized.

herbdool commented May 15, 2018

Nope, still no luck. The patch didn't fail--I tried it a second time and same result. And cleared the cache.

I used this patch https://patch-diff.githubusercontent.com/raw/backdrop/backdrop/pull/2186.patch

@serundeputy

This comment has been minimized.

Member

serundeputy commented May 15, 2018

@herbdool looks like the right patch; disable cache all together? /admin/config/development/performance

after applying the patch can you check one of the changed files for the expected change(s)? Like this one in core/includes/updater.inc.

@olafgrabienski

This comment has been minimized.

olafgrabienski commented May 15, 2018

You'll either want to start with a fresh install of Backdrop < 1.9.5 and apply the patch

@serundeputy That's what I did. Seems to be the same issue which happens to @herbdool. Going to disable cache now.

@serundeputy

This comment has been minimized.

Member

serundeputy commented May 15, 2018

i guess the removal of the .swp file is causing you folks issues with the patch.

@olafgrabienski

This comment has been minimized.

olafgrabienski commented May 15, 2018

Sounds reasonable! Anything we can change by ourselves there, or better wait for a revised patch?

@serundeputy

This comment has been minimized.

Member

serundeputy commented May 15, 2018

I'm not sure why the patch works for me and not for you or @herbdool different os?

I'm on:

geoff@yep backdrop (1.x) exit code: [0] $ sw_vers
ProductName:	Mac OS X
ProductVersion:	10.13.4
BuildVersion:	17E202

and the command i issue for the patch is:

patch -p1 < 2186.patch
@klonos

This comment has been minimized.

Member

klonos commented Jun 17, 2018

...oh you know me, I simply could not leave it alone. I have tried this quickly on ddev. Same problem and same errors in the log. I have then tested a quick and dirty "fix": I have commended this line out and replaced it with simply $info['type'] = "module";. Gave it yet another go, and everything worked just flawlessly this time!

So I guess all we have to do now is to make the installer happy with finding (or not) type = module in .info files of test modules.

@docwilmot

This comment has been minimized.

Contributor

docwilmot commented Jun 17, 2018

Someone posted an issue before: #1294

@klonos

This comment has been minimized.

Member

klonos commented Jun 29, 2018

"Someone"?? 😆

@serundeputy

This comment has been minimized.

Member

serundeputy commented Aug 11, 2018

I took a shot at a first pass at some documentation on what kinds of file permissions are required for the updates via UI to be possible are and some of the consequences. Definitely needs our docs crew to look it over and help out with wording. It is in part based on this Drupal node: https://www.drupal.org/node/244924#automated-tools-to-set-permissions pared down and tailored for Backdrop.

I nominate: @docwilmot, @jenlampton, @quicksketch, @klonos, @mikemccaffrey, @wesruv, and Jack Aponte to have a look: https://backdropcms.org/file-permissions-and-ownership

@klonos

This comment has been minimized.

Member

klonos commented Aug 14, 2018

I have done a first pass @serundeputy. A missing space here, some commas there (to make things more readable), and we're looking good 😉

its good

@herbdool

This comment has been minimized.

herbdool commented Aug 14, 2018

@serundeputy I tested this PR again. I think it works fine, though I've got a couple things that need fixing in order for it to work:

  1. The PR probably has to be squashed before merging. And if people are going to test with a patch they need to manually remove the sections with the .swp file. I know you removed it but in the patch you'll see that there is both a diff to add the .swp binary and then another diff to remove it. So I presume that squashing this will ensure that this history just disappears and there will be no mention of the .swp file.
  2. If people are testing with an old version of backdrop they should realize that older versions have line 114 in update.theme.inc line 114 wrong. It needs to reference the correct module. It should be if ($update_link && module_exists('installer')) { and NOT project_installer, which doesn't exist. The current 1.10.0 release has it right.

So a good MVP here!

@herbdool

This comment has been minimized.

herbdool commented Aug 14, 2018

@klonos before testing this again, can you check the patch file? Remove all sections relating to .swp binary and patch with that.

@jenlampton

This comment has been minimized.

Member

jenlampton commented Aug 30, 2018

I'm running this on a client site right now. So far, no issues. But I haven't tried a minor version update (only bug fixes) so I'm hesitant provide a review until after 1.11 is out.

@olafgrabienski

This comment has been minimized.

olafgrabienski commented Aug 31, 2018

On yesterday's weekly Backdrop video meeting there was a discussion if to provide core updates via UI with the 1.11 release because the feature would be much easier to test when it's in the release. In my opinion it would help a lot to have easier testing conditions. On the other hand I see a certain risk of critical issues with the core updates via UI, just because we haven't tested the feature very deeply, nor in many different hosting situations.

What about to hide the Core UI update feature somehow, or/and to give a statement in the help text that it's still in an experimental status?

@findlabnet

This comment has been minimized.

findlabnet commented Aug 31, 2018

Should this functionality to be mandatory, or can be disabled/skipped somehow?
Note: this is UX request, because normal admin is user too.

@olafgrabienski

This comment has been minimized.

olafgrabienski commented Aug 31, 2018

@findlabnet The core update via Backdrop's UI isn't mandatory. You'll still be able to update Backdrop core in the usual way, replacing the old "core" directory with a newer one, as currently described here: https://backdropcms.org/upgrade#minor-updates.

That said, when we're getting closer to provide also automatic core updates, I expect a discussion about the option how to disable / skip them - see for instance #2018 (comment).

@findlabnet

This comment has been minimized.

findlabnet commented Aug 31, 2018

So, before we sell something spare, we must to buy it?

@quicksketch

This comment has been minimized.

Member

quicksketch commented Sep 1, 2018

@olafgrabienski:

What about to hide the Core UI update feature somehow, or/and to give a statement in the help text that it's still in an experimental status?

@findlabnet:

Should this functionality to be mandatory, or can be disabled/skipped somehow?

Yes, absolutely! I had this same thought today but you two beat me to suggesting it. Let's included this core update functionality in 1.11.0 but hide it. We can enable it by default in a later release.

We already have an installer.settings.json config file. We should add a new option for enabling core updates, e.g. config_get('installer.settings', 'core_update').

In fact it would be great to have all of the various installation/updating options be able to be toggled (e.g. "module_update", "module_install", "theme_update", "theme_install", "layout_update", "layout_install"). But as we're in a time crunch, let's add the toggle for core only and expand that out later. Our primary concern here is just making it so that we can test this after release with a real example and the whole system together.

So we need the following:

  • Add core_update: false to installer.settings.json.
  • An update hook to set the default to false for existing sites.
  • A conditional check ensuring config_get('installer.settings', 'core_update') is TRUE before allowing any core updates.

At this point, I don't think any UI for toggling the option is needed.

@olafgrabienski

This comment has been minimized.

olafgrabienski commented Sep 1, 2018

At this point, I don't think any UI for toggling the option is needed.

How would I enable the feature for testing purposes: just change a configuration file?

@serundeputy

This comment has been minimized.

Member

serundeputy commented Sep 1, 2018

squashed commits to remove *.swp from git history; https://patch-diff.githubusercontent.com/raw/backdrop/backdrop/pull/2201.patch

@herbdool

This comment has been minimized.

herbdool commented Sep 1, 2018

@olafgrabienski yes, you'd have to edit the installer.settings.json file.

@herbdool

This comment has been minimized.

herbdool commented Sep 1, 2018

based on @quicksketch suggestions I've attached a patch backdrop/backdrop#2201 (comment). I could make a PR if @serundeputy can't do it before the deadline.

@herbdool

This comment has been minimized.

herbdool commented Sep 1, 2018

@serundeputy as per @quicksketch suggestion, I've created a PR backdrop/backdrop#2289

@quicksketch

This comment has been minimized.

Member

quicksketch commented Sep 1, 2018

I've tested out @herbdool's additions at backdrop/backdrop#2289 and they look great! As we've already tested this prior to the disabling option, I think we're good to go here.

I've merged in backdrop/backdrop#2289. Thanks everyone, especially @serundeputy for his direct and simple approach to this problem. This is an exciting development and I'm so glad we came up with a way to proceed on this. Woot!

@quicksketch

This comment has been minimized.

Member

quicksketch commented Sep 2, 2018

Follow-up to enable after 1.11.0 is released: #3271

@serundeputy

This comment has been minimized.

Member

serundeputy commented Sep 3, 2018

Wow! went to work on this today and it's merged! Amazing job everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment