Skip to content

Template factory check types #352

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

Closed
wants to merge 24 commits into from

Conversation

mildabre
Copy link
Contributor

@mildabre mildabre commented Jul 16, 2025

When replacing Nette\Bridges\ApplicationLatte\DefaultTemplate with a custom template class (e.g. MyTemplate) and reusing property names such as $user, but declaring them with a different type (e.g. using UserDTO instead of Nette\Security\User), the default parameter injection may cause a TypeError.

This patch adds runtime type validation to TemplateFactory, ensuring that only compatible parameters are injected into properties of custom template classes. The solution supports ReflectionNamedType and respects allowsNull(), providing safe handling for nullable types.

This PR is discussed on https://forum.nette.org/cs/36875-rfc-doplnit-typovou-kontrolu-v-templatefactory#p228757

dg and others added 23 commits September 10, 2024 12:09
- inteface Nette\Application\IRouter replaced by Nette\Routing\Router
…e() for persistent parameters. [Closes nette/nette#703][Closes nette/nette#703][Closes nette#69]" (possible BC break)

This reverts commit cda17f4.

See https://forum.nette.org/cs/35528-stejne-pojmenovany-parametr-akce-presenteru-a-persistentni-odlisne-chovani-v-nette-2-0-oproti-aktualnimu#p221742

BC break: Property must be nullable, ie: #[Persistent] public ?int $foo
…rs dynamically (BC break)

TODO: scanDir must be active
@mabar
Copy link
Contributor

mabar commented Jul 16, 2025

  • PR contains your commits with other, unrelated changes
  • Handling TypeError via try catch would be much easier, more performant and would support ReflectionIntersectionType and ReflectionUnionType as well
  • While this would be useful for the few people that use nette/security and don't want the default $user variable set, it may create a lot of confusion for anyone who'd use wrong type by accident. I had to explain to juniors what is the difference between Nette\Security\User, our overriden implementation of it and user entity a few times and this would be a headache waiting to happen. This should be at least configurable and turned off by default.

Ideal would be to use constructors for template classes and fill them ourselves, but I have no idea how to do that properly - quite a lot of internals and third-party code relies on other parts of TemplateFactory's logic. Perhaps via some applyMagic function called internally just before template render?

@mildabre
Copy link
Contributor Author

mildabre commented Jul 17, 2025

Hi Marek,

The concern that a typical developer might get confused if they accidentally declare a different type for $user in a component's typed template than what Nette injects by default should be taken seriously. If $user is mistyped, no exception is thrown, only a warning about an undefined variable, which can be easily overlooked.

The purpose of this PR was to address the ambiguity where $user in a template can mean: a) a database entity, b) Nette\Security\User, c) display data of the authenticated user.

While $user->fullName looks elegant, it still conflicts with the $user entity. So I’m leaning toward a more fundamental solution: using $user in templates strictly as an entity or ActiveRow representing database data, and choosing a different name for other cases—e.g. $authUser for Security\User, which ideally shouldn’t be exposed in templates at all. Instead, we could inject $this->template->userIdentity = $authUser->identity.

The hardcoded $user injection issue only affects templates of UI\Control components. Presenter action templates don’t have this problem, their template classes are just hints and don’t enforce types at runtime. So if someone wants to render a User entity in an action template, they can safely type $user as App\Model\User\User. But if they try to render it via a component with a typed template, they’ll hit a conflict. But this can be easy resolved with a compromise like App\Model\User\User|Nette\Security\User $user.

This issue is exclusively about the $user variable, and we don’t need to deal with union or intersection types elsewhere. That’s why I proposed a simplified solution in the second commit. The overhead from using ReflectionProperty applies only to custom templates and is negligible.

However, if developer confusion by this PR is likely to be a real issue, then it’s better not to proceed with it at all.

@dg dg closed this in eb09281 Jul 17, 2025
dg added a commit that referenced this pull request Jul 17, 2025
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.

4 participants