Skip to content

Conversation

@Jan-Schuppik
Copy link
Contributor

@Jan-Schuppik Jan-Schuppik commented Aug 22, 2025

@Jan-Schuppik Jan-Schuppik requested a review from nilmerg August 22, 2025 13:51
@Jan-Schuppik Jan-Schuppik self-assigned this Aug 22, 2025
@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Aug 22, 2025
@Jan-Schuppik Jan-Schuppik force-pushed the feature/http-api-new-approach branch from 9a3a6aa to aa2b625 Compare August 25, 2025 12:48
Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

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

This review covers only the early parts of routing and dispatching.

@nilmerg
Copy link
Member

nilmerg commented Aug 26, 2025

Oh, and I got the postgres tests working in my environment. Please get back to me for this.

Copy link
Contributor

@sukhwinder33445 sukhwinder33445 left a comment

Choose a reason for hiding this comment

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

I have looked at channels and contact tests so far, next I will look at contactgroup tests.

Copy link
Contributor

@sukhwinder33445 sukhwinder33445 left a comment

Choose a reason for hiding this comment

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

Please add following changes as well:

  • The PHPDoc for the method must contain a newline between different annotations, i.e. @param, @return.
  • Update method PHPDoc if required.
  • Make the getPlural() method protected.
  • Remove the resolved todos.

@Jan-Schuppik Jan-Schuppik force-pushed the feature/http-api-new-approach branch from 30b0b1b to da665ca Compare September 26, 2025 09:01
Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

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

Let's discuss how we handle the openapi description and the usage of Swagger, as there are three possible solutions for me:

  • Caching (properly, not the way it's right now ;) )
  • CI (as your todo suggests)
  • Static (it's a versioned api after all)

Copy link
Member

Choose a reason for hiding this comment

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

Missing license header. It seems, not only this file is missing it, please make sure all PHP files have one.

@Jan-Schuppik Jan-Schuppik force-pushed the feature/http-api-new-approach branch from 3353654 to 9376563 Compare October 31, 2025 13:22
Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

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

🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉

@nilmerg nilmerg merged commit 17cf24a into main Oct 31, 2025
7 of 10 checks passed
@nilmerg nilmerg deleted the feature/http-api-new-approach branch October 31, 2025 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla/signed CLA is signed by all contributors of a PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants