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

Support Nextcloud 30, update CSP header #1336

Merged
merged 6 commits into from
Nov 3, 2024
Merged

Support Nextcloud 30, update CSP header #1336

merged 6 commits into from
Nov 3, 2024

Conversation

Ma27
Copy link
Contributor

@Ma27 Ma27 commented Oct 6, 2024

Closes #1333
It probably makes most sense to review this commit-by-commit.

cc @come-nc @susnux @tacruc

css/style.scss Outdated Show resolved Hide resolved
lib/Controller/ContactsController.php Show resolved Hide resolved
lib/Controller/ContactsController.php Outdated Show resolved Hide resolved
tests/bootstrap.php Show resolved Hide resolved
@Ma27
Copy link
Contributor Author

Ma27 commented Oct 7, 2024

Btw I've seen that the CI failed because of a missing Signed-off-by, will fix it up on the next iteration.

Ma27 added 3 commits October 7, 2024 17:26
Closes nextcloud#1333

I decided to not cross-check the availability of all all deprecated
things with older majors, so both min and max version are v30 for now.

A list of all things that I've fixed:

* Deprecated SCSS variables updated as documented in the upgrade notes[1].
* Fixed a bunch of query builder deprecations:
  * $qb->resetQueryParts() is deprecated, one should just create a new
    query-builder object. Done that.
  * Calling `$qb->expr()->andX()` (and `orX()`) with no arguments is
    deprecated. Instead, one should list all arguments in an array and
    pass that to the method. Reworked queries where necessary to do so.
* The PHPUnit version I got by running `make test` downloaded a version
  that prohibits non-static data providers. Made all affected data
  providers static.

With that, everything from `make test` passes in a local dev
installation of Nextcloud 30 except for two tests:

    1) OCA\Maps\Controller\PhotosControllerTest::testAddGetPhotos
    Failed asserting that actual size 0 matches expected size 1.

    /home/ma27/Projects/nextcloud/apps/maps/tests/Unit/Controller/PhotosControllerTest.php:206

    2) OCA\Maps\Controller\TracksControllerTest::testAddGetTracks
    Failed asserting that false matches expected true.

    /home/ma27/Projects/nextcloud/apps/maps/tests/Unit/Controller/TracksControllerTest.php:205

Both tests failed on the previous commit already, so I decided to leave
those for now.

[1] https://docs.nextcloud.com/server/latest/developer_manual/app_publishing_maintenance/app_upgrade_guide/upgrade_to_30.html#front-end-changes

Signed-off-by: Maximilian Bosch <maximilian@mbosch.me>
This is already set by Nextcloud's bootstrap (and from what I've seen we
require that anyways), so no need to do this on our own.

This also resolves the following warnings from `make test`:

    Warning: Constant PHPUNIT_RUN already defined in /home/ma27/Projects/nextcloud/tests/bootstrap.php on line 7
    PHP Warning:  Cannot modify header information - headers already sent by (output started at /home/ma27/Projects/nextcloud/tests/bootstrap.php:7) in /home/ma27/Projects/nextcloud/lib/private/legacy/OC_Response.php on line 71

Signed-off-by: Maximilian Bosch <maximilian@mbosch.me>
So far, we only allowed subdomains, but when I rolled out the other
changes, this was missing from having a working map with OSM tiles.

This is probably not related to the other changes (it worked fine a few
days ago, perhaps something was just changed within OSM), but I figured
it's still worth fixing.

Signed-off-by: Maximilian Bosch <maximilian@mbosch.me>
@AndyScherzinger
Copy link
Member

cc @julien-nc I guess

Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

All good apart from this one forgotten getQueryBuilder call.

Ideally calls to execute should be replaced with executeQuery/executeStatement but that’s not removed in 30 yet so it’s unrelated to this PR.

lib/Service/AddressService.php Show resolved Hide resolved
Rather than having to remember to reset the query builder in each method
that uses it, these are now created on-demand from the db connection.

Signed-off-by: Maximilian Bosch <maximilian@mbosch.me>
@tacruc
Copy link
Collaborator

tacruc commented Nov 1, 2024

Hi @Ma27,
Thanks a lot for your contribution I appreciate it a lot.
Unfortunately I'm currently quite busy. Right now I'm sitting in a train and wanted to use the time to update and merge this. Unfortunately my laptop run out of battery and the plugs are not working.
So I'm stuck with my phone.

Could you update the Test workflow such that the tests are run on NC30?
https://github.com/nextcloud/maps/blob/master/.github/workflows/test.yml

@Ma27
Copy link
Contributor Author

Ma27 commented Nov 1, 2024

Hi @tacruc!
No worries at all!

Pushed a commit that does that. Not sure if it's completely correct though, I usually work with other CI systems 🙃
However, I think you should be able to push to my fork's branch as well, right?

@tacruc
Copy link
Collaborator

tacruc commented Nov 3, 2024

Thanks a lot. In theory I should be able to push to your branch. In practice I didn't find a good git client for Android. Do you know one?

Can you remove v28 and v29 from testing and make the DCO, happy? I think then we can go for a merge.

@Ma27
Copy link
Contributor Author

Ma27 commented Nov 3, 2024

Done.

Signed-off-by: Maximilian Bosch <maximilian@mbosch.me>
@tacruc tacruc enabled auto-merge November 3, 2024 12:34
@tacruc tacruc merged commit 97060f8 into nextcloud:master Nov 3, 2024
17 of 18 checks passed
@Ma27 Ma27 deleted the nc30 branch November 3, 2024 12:50
@tacruc
Copy link
Collaborator

tacruc commented Nov 3, 2024

@Ma27, there is a nightly build with your PR https://apps.nextcloud.com/apps/maps

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.

Please make the app compatible with NC30
4 participants