Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix/app fetcher php compat comparison #25335

Merged
merged 2 commits into from
Jan 27, 2021
Merged

Fix/app fetcher php compat comparison #25335

merged 2 commits into from
Jan 27, 2021

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Jan 26, 2021 •
edited
Loading

Reverts bedd9ac.

This is how #24416 was meant to work.

Fixes nextcloud/mail#4383

Note to testers: there is some caching that might make it impossible to see the fix immediately. Apply the changes as patch from https://patch-diff.githubusercontent.com/raw/nextcloud/server/pull/25335.patch, give the cache an hour or so to expire and try again

@ChristophWurst
Revert "Fix comparison of PHP versions"
652bf9d 
This reverts commit bedd9ac.
@ChristophWurst
Fix app fetcher php comparison to allow wider ranges
b4f71cc 
When app app specifies php 7.4 as upper limit we have to allow the
installation on php>7.4.0. The previous version check didn't do that.
This adjusts the regexes to discard any irrelevant suffix after the
three version numbers so that we can use more fine granular checks than
php's version_compare can do out of the box, like for php 7.4 we only
compare the major and minor version numbers and ignore the patch level.

Signed-off-by: Christoph Wurst <[email protected]>
@ChristophWurst ChristophWurst added this to the Nextcloud 21 milestone Jan 26, 2021
@ChristophWurst ChristophWurst self-assigned this Jan 26, 2021
@ChristophWurst ChristophWurst mentioned this pull request Jan 26, 2021
Closed
kesselb
kesselb approved these changes Jan 26, 2021
View reviewed changes
Copy link
Contributor

@kesselb kesselb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

nickvergessen
nickvergessen approved these changes Jan 26, 2021
View reviewed changes
tests/lib/App/CompareVersionTest.php
['13.0.1-beta.1', '13', '<=', true],
['7.4.14', '7.4', '<=', true],
['7.4.14-ubuntu', '7.4', '<=', true],
['7.4.14-ubuntu', '7.4.15', '<=', true],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also have a false test with Ubuntu

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#25348

@rullzer rullzer merged commit 3a9c7f9 into master Jan 27, 2021
@rullzer rullzer deleted the fix/app-fetcher-php-compat-comparison branch January 27, 2021 14:14
@rullzer rullzer mentioned this pull request Jan 27, 2021
Merged
19 tasks
@szaimen
Copy link
Contributor

szaimen commented Jan 27, 2021

Will this get backported to NC20?

@ChristophWurst
Copy link
Member Author

/backport to stable20

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Jan 27, 2021
Merged
This was referenced Jan 27, 2021
Merged
Closed
Open
Closed
Closed
@ChristophWurst ChristophWurst mentioned this pull request Jan 28, 2021
Closed
@ManafxMT
Copy link

Hello Christoph
I have the same problem - Email disappered since my update to 20.0.6.

How to implement the patch please?
Do you have a link where I can read about the procedure?

I never had an issue like this before.

@ChristophWurst ChristophWurst mentioned this pull request Jan 28, 2021
Closed
@ChristophWurst
Copy link
Member Author

Let me show you a magic trick. The PR URL is https://github.com/nextcloud/server/pull/25335. Now append a .patch and you will be redirected to https://patch-diff.githubusercontent.com/raw/nextcloud/server/pull/25335.patch. Download that patch.

Then you can use your patch command to apply the changes: https://www.cyberciti.biz/faq/appy-patch-file-using-patch-command/.

@vladlucky
Copy link

vladlucky commented Jan 28, 2021 •
edited
Loading

Having the same issue as described here by others - unable to locate/enable the TOTP app in Nextcloud.

I've tried to apply the patch but am getting the following response. I have a brand new installation of Nextcloud 20.0.6.1 with Ubuntu 20.04.1 LTS and PHP 7.4.14.

I am a total nub to Nextcloud administration and git, this was my first build so any help will be much appreciated.

image

@ManafxMT
Copy link

Do besides being a developer you are a magician too 👍💐
First, thanks for your quick response.
Let me understand: if not installed, I need to install "patch".
After that, in the terminal I only put
Patch (downloadpath.patch)?
With or without sudo?
Is there a chance to messup my whole installation- so shall I do a backup before?
I found some videos around the patch command, but still not sure...

@ChristophWurst
Copy link
Member Author

patch -p1 < 25335.patch should be the command. No sudo required although you possible want to run this as your web server user, like sudo -u www-data patch … on Debian/Ubuntu.

@ManafxMT
Copy link

Turbo response, thanks.
Before I try it: when will be the app available in the app section on 20.06 again?
Maybe I better wait for it...

@ChristophWurst
Copy link
Member Author

it looks like we can release 20.0.7 next week, but I can't promise it

@ManafxMT
Copy link

Cool, I think I better wait - if there is a slight chance to mess up the patch - I will manage. (joking).
Thanks for your time and explainations...will be very helpful for others for sure.

@vladlucky
Copy link

Thank your Christoph, the patch resolved the issue and I am now able to install the TOTP plug-in!

@anoymouserver anoymouserver mentioned this pull request Jan 30, 2021
Closed
3 tasks
@vladlucky
Copy link

Although TOTP is working after applying the patch, I started seeing the following errors. In addition to seeing the errors, the app store is not always accessible - sometimes it loads sometimes it doesn't:

image

image

image

@Rello Rello mentioned this pull request Feb 1, 2021
Closed
@ChristophWurst
Copy link
Member Author

the app store was down, that's why you saw a "new" exception

@pachulo pachulo mentioned this pull request Feb 2, 2021
Closed
@natrius
Copy link

natrius commented Feb 2, 2021

Just to clarify: This seems to resolve the problem with extracting Nextcloud News as well? nextcloud/news#1086

@ManafxMT
Copy link

ManafxMT commented Feb 3, 2021

I installed NC 20.0 7 and the Email App - all works great, thanks a lot.
It is reacting faster then before too.

Only it is not keeping my signature - can I do something about it?

@ManafxMT
Copy link

ManafxMT commented Feb 3, 2021

Managed - even with the signature.
Total happy now.

You are doing an awesome job 👍👍

@ChristophWurst
Copy link
Member Author

Just to clarify: This seems to resolve the problem with extracting Nextcloud News as well? nextcloud/news#1086

nextcloud/news#1086 (comment).

Only it is not keeping my signature - can I do something about it?

Known regression but feel free to report is as bug for better tracking.

✌️

@fischer-felix
Copy link

patch -p1 < 25335.patch should be the command. No sudo required although you possible want to run this as your web server user, like sudo -u www-data patch … on Debian/Ubuntu.

How would you apply this patch when using a nextcloud snap installation?

@ChristophWurst
Copy link
Member Author

How would you apply this patch when using a nextcloud snap installation?

Please ask on https://help.nextcloud.com/c/support/7

@ChristophWurst ChristophWurst mentioned this pull request Feb 3, 2021
Closed
@kyrofa kyrofa mentioned this pull request Feb 4, 2021
Closed
@DennisWilken
Copy link

Hi Christoph, thanks for your patch. I am trying to install it on a Docker container. But I am not quite sure how to apply it. I downloaded it using curl https://patch-diff.githubusercontent.com/raw/nextcloud/server/pull/25335.patch -o 25335.patch
and tried to install it using patch -p1 < 25335.patch. Next, I am asked for the file to patch:

image

Do I have to run the patch in a certain folder? What path do I have to enter? Am I doing anything else wrong? Thanks for your help!

@szaimen
Copy link
Contributor

szaimen commented Feb 7, 2021

@DennisWilken just update your NC to 20.0.7 (it is included with this version). There is no need to apply the fix manually.

@DennisWilken
Copy link

@DennisWilken just update your NC to 20.0.7 (it is included with this version). There is no need to apply the fix manually.

Version 20.0.7 is not yet available on docker (see here) So I can either wait, patch or manually upgrade.

@sokai sokai mentioned this pull request Feb 8, 2021
Closed
@Potherca Potherca mentioned this pull request Apr 7, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen approved these changes

@kesselb kesselb kesselb approved these changes

@rullzer rullzer Awaiting requested review from rullzer

Assignees

@ChristophWurst ChristophWurst

Labels
3. to review Waiting for reviews bug feature: apps management regression
Projects
None yet
Milestone
Nextcloud 21
Development

Successfully merging this pull request may close these issues.

Mail app not available for installation
10 participants
@ChristophWurst @szaimen @ManafxMT @vladlucky @natrius @fischer-felix @DennisWilken @nickvergessen @kesselb @rullzer