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

feat: lift min PHP version and NC #2687

Merged
merged 1 commit into from
Oct 14, 2024

Conversation

SMillerDev
Copy link
Contributor

  • Resolves: #

Summary

Update some PHP usage and update NC usage

Checklist

@SMillerDev SMillerDev force-pushed the fix/deprecation branch 3 times, most recently from 16cc083 to a7a2c23 Compare August 22, 2024 08:24
@SMillerDev SMillerDev force-pushed the fix/deprecation branch 9 times, most recently from f6e268d to 6677da5 Compare September 17, 2024 16:52
@Grotax
Copy link
Member

Grotax commented Sep 21, 2024

rebased

@Grotax
Copy link
Member

Grotax commented Sep 21, 2024

Hey, I tried to look into the failing tests but got stuck at some point.

One of my findings was that in the search providers we have

        if (method_exists($query, 'getFilter')) {
            $term = $query->getFilter('term')?->get() ?? '';
        } else {
            $term = $query->getTerm();
        }

Which is no longer needed nc29+ versions will all have the getFilter method so also in the test the getfilter method is called and not getTerm don't know how to mock the result though.

Also looked again into the issue with the deprecated method withConsecutive can't find any proper replacement.

@SMillerDev
Copy link
Contributor Author

Yeah, I fixed the search provider in one place. Let me see if I can do it in others too.

@SMillerDev SMillerDev force-pushed the fix/deprecation branch 2 times, most recently from e8f47ab to c657e46 Compare October 13, 2024 13:18
@SMillerDev
Copy link
Contributor Author

Okay, now all deprecations should be fixed.

@SMillerDev SMillerDev force-pushed the fix/deprecation branch 7 times, most recently from e8e1955 to 392144d Compare October 13, 2024 14:05
@Grotax Grotax merged commit 4e583b7 into nextcloud:master Oct 14, 2024
22 of 24 checks passed
@brunob
Copy link
Contributor

brunob commented Jan 22, 2025

Nice, but it feel strange to require PHP 8.2 as min version since nextcloud 29 allow PHP 8.1 cf https://docs.nextcloud.com/server/29/admin_manual/installation/system_requirements.html

@SMillerDev
Copy link
Contributor Author

It's the minimum recommended version, and I don't really have time to volunteer for support of old PHP versions.

@brunob
Copy link
Contributor

brunob commented Jan 22, 2025

I fully understand, i was just pointing that it's strange than an app doesn't follow the same requirement of the core :)

@SMillerDev
Copy link
Contributor Author

There's really no connection between core and this app, so it's not that odd we're setting our own standards

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.

3 participants