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

refactor(router): convert route attributes to plain objects #822

Merged
merged 5 commits into from
Dec 7, 2024

Conversation

blackshadev
Copy link
Contributor

What

@aidan-casey made an alternative suggestion to removing the Route as abstract base. With this rework we also lose the IsRoute trait and all Route builder "verbs" (Get, Post, etc) become plain old php objects. No method, no logic, no magic. This will make it easier to use the Route interface. But it will make it "harder" to do logic in these Route builder "verbs" as you do not have any method to hook into, you should do everything in the constructor.

Personally, I think this is as clean as it gets.

DO NOT MERGE

Based on #821 as PHPStan 2.0 is required, the old one will panic at the sight of property hooks in the interface

@blackshadev blackshadev changed the title feat(router): Route attribute rework v2 feat(router): route attribute rework v2 Dec 6, 2024
@innocenzi innocenzi changed the title feat(router): route attribute rework v2 refactor(router): convert route attributes to plain objects Dec 6, 2024
@aidan-casey
Copy link
Member

Cool PR!

@brendt
Copy link
Member

brendt commented Dec 7, 2024

Nice!

@blackshadev
Copy link
Contributor Author

PHPStan seems drunk, can someone rerun it? It reports an error of unnecessary null safe, while on that line (and even in the file) no nullsafe operators are used...

@aidan-casey
Copy link
Member

@blackshadev I reran it from mobile, but same error. What file/line is it complaining about? It's not complaining locally?

@blackshadev
Copy link
Contributor Author

@blackshadev I reran it from mobile, but same error. What file/line is it complaining about? It's not complaining locally?

It is not, but I found out why. Somebody other than me rebased this branch from main , which contained the new error. I rebased the phpstan-v2 branch and added the error to the baseline. I will rebase this one to phpstan-v2 and it should be green.

@brendt brendt merged commit 88bd85d into tempestphp:main Dec 7, 2024
60 checks passed
@brendt
Copy link
Member

brendt commented Dec 7, 2024

Very nice!

@brendt
Copy link
Member

brendt commented Dec 7, 2024

Do we need to update the docs as well?

@blackshadev blackshadev deleted the http-route-rework-v2 branch December 7, 2024 19:44
@blackshadev
Copy link
Contributor Author

Do we need to update the docs as well?

A bit. I can do it tomorrow.

@blackshadev
Copy link
Contributor Author

Do we need to update the docs as well?

updated docs in this PR

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