Fix plugins might not be updated when updating core #16322
Conversation
|
|
||
| if (!empty($response)) { | ||
| $messages = array_merge($messages, $response); | ||
| $cliMulti = new CliMulti(); |
There was a problem hiding this comment.
not 100% sure it's a good idea using CliMulti but reckon it might be beneficial in case there Matomo can't be easily reached through the internet. This way it might cause problems only for those where async using CLI doesn't work and the Matomo can't be reached through the internet.
There was a problem hiding this comment.
eg in my case the original http call wouldn't have worked because:
Got invalid response from API request: https://.... Response was 'curl_exec: SSL certificate problem: unable to get local issuer certificate. Hostname requested was: ...'
plugins/CoreUpdater/Updater.php
Outdated
| $response = array_shift($responses); | ||
| $response = @json_decode($response, $assoc = true); | ||
| if (!empty($response) && is_array($response)) { | ||
| $messages = array_merge($messages, $response); |
There was a problem hiding this comment.
one problem here is that any error during oneClickUpdatePartTwo won't trigger an exception and therefore no error will be set here https://github.com/matomo-org/matomo/blob/3.14.1-b1/plugins/CoreUpdater/Controller.php#L167 and therefore the user won't see any of the messages should something go wrong. This was already a problem with the original solution but don't know if we want to show error messages for this action or simply ignore them as it affects only marketplace plugins? And they could simply update them afterwards
There was a problem hiding this comment.
Do you mean a fatal error? Or something else? If it's a fatal error, perhaps the safemode hook could be used. Not sure if it's worth the effort though.
There was a problem hiding this comment.
@diosmosis I meant eg if there's an error like I had it Got invalid response from API request: https://.... Response was 'curl_exec: SSL certificate problem: unable to get local issuer certificate. Hostname requested was: ...' or if there's suddenly a bug with the nonce and the call is not actually working etc. Before the token_auth was missing so the action for the 2nd step would have always returned an error but the error was never shown.
There was a problem hiding this comment.
I guess we'd have to handle this when the controller action is called? Ie, "cannot parse json response, response is: curl-exec: ..." or "invalid token, could not complete install". or maybe i'm missing something?
There was a problem hiding this comment.
Note: I don't know if it's actually that important.
| if (empty($nonce)) { | ||
| return json_encode(['no token']); | ||
| } | ||
| $value = Option::get('NonceOneClickUpdatePartTwo'); |
There was a problem hiding this comment.
not using a constant here as depending how they upgrade or code changes maybe it wouldn't exist. could use one though
|
left one comment, otherwise lgtm |
…if climulti doesnt work
| @@ -151,6 +182,12 @@ public function oneClickUpdatePartTwo() | |||
| { | |||
| $messages = []; | |||
|
|
|||
| if (!Marketplace::isMarketplaceEnabled()) { | |||
There was a problem hiding this comment.
had marketplace disabled locally and it was actually before resulting in below error but because we didn't show messages we never noticed basically
| @@ -36,6 +36,17 @@ | |||
| </div> | |||
| </div> | |||
|
|
|||
| <h2>{{ 'CoreUpdater_UpdateLog'|translate }}</h2> | |||
There was a problem hiding this comment.
now showing a log of messages... this can help troubleshoot issues and would show a message if for example plugins were disabled during the update etc.
|
@diosmosis updated the PR. Noticed the second step was actually failing when marketplace was disabled. Should something go wrong because user has invalid SSL falling back to execute the second action within the same request. This should usually work quite well especially between minor and patch releases. And instead of any special error handling I reckon it's actually better to simply always show the log output in the success screen as well. So people notice eg if plugins were disabled. See example in https://builds-artifacts.matomo.org/matomo-org/matomo/3.x-dev/42112/OneClickUpdate_update_success.png Note that the translation key appears and I think that's because of some caching as I think it updates from the latest stable where the translation key was not present yet. Locally this works for me. See https://builds-artifacts.matomo.org/matomo-org/matomo/3.x-dev/42112/OneClickUpdate_update_success.png Also note that the first tick is a bit moved to the right which seems to. be because it's the first line right after |
Seems to have regressed in #15770
Haven't tested it yet fully but this should work. We'll then also need to merge this into Matomo 4. The previous token_auth check wouldn't work there because it would check against the new app specific token auth table which wouldn't exist yet at the time this is executed. Therefore using an nonce which is better in general anyway so the action can be only called when initiated by the first update step.
Unfortunately this will basically only work on the next update. So if people update to 3.14.1 it will only work when they update from that version. It won't work yet from 3.14.0 to 3.14.1 since the old code would be used there.