-
Notifications
You must be signed in to change notification settings - Fork 279
Use attribute to render templates #3145
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
base: main
Are you sure you want to change the base?
Conversation
1e1534e
to
ec6e700
Compare
I'm not sure this is cleaner. Instead of explicitly calling the template this is now all written in meta-annotation. Also, now the return type is a mix of array and Response. What's the gain? |
For me it's twofold:
However if the majority doesn't like it I'm fine with not doing it |
Maybe I'm overlooking something, but which branches? The difference I basically see it that instead of explicitly calling
Well, but that's just written down in the annotation, so now that's written in two places and if you forget to update the annotation, then you still have a bug. So I don't really see the advantage of that. |
See for example here: https://github.com/DOMjudge/domjudge/pull/3145/files#diff-cc71aa6a9b851ee37894e1943a11fd1d6676ae347fac6e088ec93325e4a90e1aL93-R111
This is correct.
Well, that's how PHP works I guess. Instead of arrays we could have data objects everywhere, which makes this 'link' strict, but adds a lot of classes. Again, I see your point. I still like the new way better, but if you and others disagree we will just close this PR, I'm fine with that. |
I have a slight preference for the new code. Do we have any checks that the annotations are correct? |
We don't and I wonder how we could do that. We somehow have to know what variables are expected in the Twig template. We could write something ourselves. A quick Google gave me https://github.com/twigstan/twigstan which is ' |
I prefer the old version as it's more KISS. |
I saw that since Symfony 6.2 you can use a
#[Template]
attribute to indicate which template to render. I think it makes the controllers a bit cleaner, so I wanted to implement it.Then I found out that we have some controllers that render a different file for AJAX requests, so I added a
#[AjaxTemplate]
attribute to support this. I also added a way to set a custom response object, which we sometimes do for setting custom headers or other things.I then modified the public controller myself and afterwards used AI (Claude in particular) to rewrite all other controllers and verified myself afterwards if it did anything weird (like long lines or stuff that just doesn't work).
I committed the the new attribute + event listeners in a separate commit. As said the big, second one is mostly AI generated.
It does add some lines to make the docblocks clearer. If we don't like that, we can change all of them to
array<string, mixed>
.