Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Open
nickygerritsen wants to merge 2 commits into DOMjudge:main
base: main
Choose a base branch
Loading
from nickygerritsen:template-attribute

Conversation

@nickygerritsen
Copy link
Member

@nickygerritsen nickygerritsen commented Oct 7, 2025

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>.

Copy link
Member

eldering commented Oct 7, 2025

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?

Copy link
Member Author

For me it's twofold:

  • The Ajax ones have less if branches
  • We are more explicit in the array shape return type

However if the majority doesn't like it I'm fine with not doing it

Copy link
Member

eldering commented Oct 9, 2025

For me it's twofold:

* The Ajax ones have less if branches

Maybe I'm overlooking something, but which branches?

The difference I basically see it that instead of explicitly calling render and return a Reponse, this is now encoded in annotation and the annotation also declares what template data should be returned.

* We are more explicit in the array shape return type

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.

Copy link
Member Author

For me it's twofold:

* The Ajax ones have less if branches

Maybe I'm overlooking something, but which branches?

See for example here: https://github.com/DOMjudge/domjudge/pull/3145/files#diff-cc71aa6a9b851ee37894e1943a11fd1d6676ae347fac6e088ec93325e4a90e1aL93-R111

The difference I basically see it that instead of explicitly calling render and return a Reponse, this is now encoded in annotation and the annotation also declares what template data should be returned.

This is correct.

* We are more explicit in the array shape return type

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.

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.

Copy link
Member

I have a slight preference for the new code.

Do we have any checks that the annotations are correct?

Copy link
Member Author

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 '
very experimental'.

Copy link
Member

I prefer the old version as it's more KISS.

Copy link
Member

vmcj commented Oct 27, 2025

I think I would like it more if we can return a DTO for it. I like how this forces us to provide the same information to both the AJAX & non-AJAX templates but this should be a PR which removes lines of code but it seems it just adds lines of code.

eldering reacted with thumbs up emoji

Copy link
Member Author

Of course most added lines are added because we now specify what types we return. If we replace them with array<string, mixed> this would not be the case.

But I agree that it would be better if we had DTO's (though this would add even more lines).

However this is not possible with the native Symfony way of doing this. We could create a custom event to do this, but then I wonder if it's really worth it.

I'll leave this PR open for now, we can probably discuss in Karlsruhe whether to close it or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /