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.

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

Use new types introduced in Nextcloud 21 and bump compatibility #173

Merged
merged 2 commits into from
Apr 10, 2021

Conversation

hlnd
Copy link
Contributor

@hlnd hlnd commented Mar 9, 2021

Signed-off-by: Ole Morten om@haaland.biz

Fixes #165.

Changes proposed in this pull request:

  • Use new types.
  • Bump compatibility: I'm not intimately familiar with Nextcloud's APIs, but I suppose using this new class is going to break compatibility with Nextcloud <21.

Signed-off-by: Ole Morten <om@haaland.biz>
Copy link

@brknkfr brknkfr left a comment

Choose a reason for hiding this comment

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

Seems to work fine with a new nextcloud 21 installation.

@brknkfr brknkfr mentioned this pull request Mar 18, 2021
@violoncelloCH
Copy link
Member

Thanks for the PR @hlnd
@ChristophWurst could you take a look if those changes make sense and if there is nothing missing?
(That way I could hopefully create a new release next week after this is merged)

CI is failing because of #159 (which is already out of date again)

@almereyda
Copy link

After checking the patch, I think it might not be the best idea to change an existing migration.

On the other hand were the DBAL types removed with 21, and that migration couldn't possibly run anymore.

But will this patch also allow to take care of clients, which have existing users_external tables already initialized with Type::String, or is this kind of type migration taken care of by the NC21 upgrade migration?

@ChristophWurst
Copy link
Member

The migration will still be run for new migrations. Existing migration files have to be touched.

@doobry-systemli
Copy link

I can verify that the app works as expected with Nextcloud 21.

Would be nice if this PR could be merged and a new app release be published @violoncelloCH 😊

Signed-off-by: Jonas Sulzer <jonas@violoncello.ch>
@violoncelloCH violoncelloCH merged commit fa1e714 into nextcloud:master Apr 10, 2021
@violoncelloCH
Copy link
Member

thanks for the PR @hlnd !

@oriolo
Copy link

oriolo commented Apr 10, 2021

Well done, It works as expected.
Just two minor issues to report with this configuration:
'<imap_server>', , '<ssl/tls>', ', true, true
the user is created but no group is defined for him nor the email address.
Just in case I would also like to suggest to add a 7th parameter in which one can define the quota limit.
Thank you.

@ychaouche
Copy link

Thank you all. The red enable untested app has now disappeared from the admin pannel > apps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Breaking changes in Nextcloud 21
8 participants