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

chore: migrate some routes to the new format #2873

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SMillerDev
Copy link
Contributor

  • Resolves: #

Summary

Checklist

@SMillerDev SMillerDev force-pushed the chore/controller/new_routes branch 3 times, most recently from 7cc727f to 73b0d53 Compare November 13, 2024 11:56
@Grotax
Copy link
Member

Grotax commented Nov 13, 2024

It looks like the cleanup path does not work.

When calling the cleanup url

http --ignore-stdin -b -a ${user}:${APP_PASSWORD} GET http://localhost:8080/index.php/apps/news/api/v1-2/cleanup/after-update

I see 405 error on the server.

@SMillerDev
Copy link
Contributor Author

That's odd, can you post the log or the output?

@Grotax
Copy link
Member

Grotax commented Nov 13, 2024

Nothing in the logs.

PHP server logs

[Wed Nov 13 15:19:55 2024] 127.0.0.1:37036 Accepted
[Wed Nov 13 15:19:56 2024] 127.0.0.1:37036 [405]: GET /index.php/apps/news/api/v1-2/cleanup/after-update
[Wed Nov 13 15:19:56 2024] 127.0.0.1:37036 Closing

The route normally returns null but now of course nothing.

Since 405 means method not allowed I assume the route is not matched like you expected.
Lucky that the update tests catch that because they are not intended to test the api it was just easier to implement the update tests that way.

@SMillerDev SMillerDev marked this pull request as draft November 13, 2024 15:08
@SMillerDev
Copy link
Contributor Author

Marking this as draft and I'll investigate later.

@SMillerDev SMillerDev force-pushed the chore/controller/new_routes branch 2 times, most recently from 43898a7 to 6e87c5c Compare November 16, 2024 14:56
@SMillerDev SMillerDev force-pushed the chore/controller/new_routes branch from 6e87c5c to f7a2959 Compare November 16, 2024 14:57
@SMillerDev
Copy link
Contributor Author

I think we can't use the ApiRoute because the way we implement our API routes predates that concept. I'll see if I can adapt it, but if it would change the routes I'm not sure if we want that.

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