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

Add templateType & varType to some Admin presenters #72

Merged
merged 3 commits into from
Jan 18, 2023

Conversation

spaze
Copy link
Owner

@spaze spaze commented Jan 18, 2023

No description provided.

@spaze spaze self-assigned this Jan 18, 2023
@spaze spaze merged commit 526d29d into main Jan 18, 2023
@spaze spaze deleted the spaze/template-types branch January 18, 2023 23:29
@lulco
Copy link
Contributor

lulco commented Jan 23, 2023

Are these Template classes really used by nette in presenters? Or $this->template is still DefaultTemplate? {varType} / {templateType} are not transfered to compiled template yet, but I can imagine we can analyse them if they will be used in presenter too. e.g. You call $this->createTemplate(ClassName) or use correct namespace for those template classes (not sure now, I have to check)

@lulco
Copy link
Contributor

lulco commented Jan 23, 2023

But there should be no need for it because all variables should be collected from presenter anyway, if you have some use case when they are not collected, please create issue, ideally with failing test case

@spaze
Copy link
Owner Author

spaze commented Jan 23, 2023

Are these Template classes really used by nette in presenters?

No, they're used by Latte, or more like my IDE. See https://phpfashion.com/phpstorm-a-napovidani-nad-this-template

Or $this->template is still DefaultTemplate

Yes, $this->template in presenters is a DefaultTemplate instance.

{varType} / {templateType} are not transfered to compiled template yet,

I've noticed, there's also an issue mentioned in #71 but these classes were not added (at least not primarily) for the PHPStan Latte extension, there's no difference if they're there, or not.

if you have some use case when they are not collected, please create issue, ideally with failing test case

Yeah, don't worry, I'm filing bugs if I see one :-)

@spaze
Copy link
Owner Author

spaze commented Jan 23, 2023

Just checked (once again) why I still have

/**
 * @property-read DefaultTemplate $template
 */

in my presenters and not for example the class added here, like:

/**
 * @property-read MichalSpacekCz\Admin\Presenters\PresenterTemplates\Interviews\InterviewsDefault $template
 */

and that's because the InterviewsDefault class describes just one action, default in this case, and not all variables assigned in the whole presenter. I'm not sure what's the best way to do it at the moment.

@lulco
Copy link
Contributor

lulco commented Jan 24, 2023

Those templates are little bit tricky. They are not important for phpstan-latte yet, but we are discussing about them.

This could be helpfull https://github.com/nette/application/blob/v3.1/src/Application/UI/Presenter.php#L577

Template class have to be in the same namespace as Presenter to be found automatically. And you can create template for each actions - InterviewPresenter could have InterviewDefaultTemplate, InterviewDetailTemplate etc. See code above.

You can also create your template class in action / render (not both! because it would be reset - you will lost data assigned in action):
$this->template = $this->createTemplate(ClassName);

@spaze
Copy link
Owner Author

spaze commented Jan 24, 2023

This could be helpfull

Oh, nice! I'll check this but it seems this is what I was looking for, thanks! Would introduce quite some new files in the directory where presenters are now but I guess I can live with that.

Just a minute ago I found out something similar but they mentioned ClassTemplate, not ClassActionTemplate.

$this->template = $this->createTemplate(ClassName);

This may also be useful especially for cross-references and clicking in IDE, I'll see which one I'll go with. Thanks.

@lulco
Copy link
Contributor

lulco commented Jan 24, 2023

Yeah, it's probably not documented. I know about it just because it was my PR :D

@spaze
Copy link
Owner Author

spaze commented Jan 24, 2023

🤣 Nice, thanks! I'll see if I can write the missing documentation, or at least mention it somewhere in the docs.

(Just for my info, this is the PR nette/application#277)

@spaze
Copy link
Owner Author

spaze commented Jan 25, 2023

You can also create your template class in action / render (not both! because it would be reset - you will lost data assigned in action):
$this->template = $this->createTemplate(ClassName);

Fyi, seems like you can't do this because template is marked as @property-read in https://github.com/nette/application/blob/d53d267f4c568bf848d2381b2c2aa28c580c1ed2/src/Application/UI/Control.php#L18
and Nette throws something about member access. Re-adding the property in a child class with just @property doesn't work for me. It would probably be my preferred "explicit" way.

@lulco
Copy link
Contributor

lulco commented Jan 25, 2023

You can also create your template class in action / render (not both! because it would be reset - you will lost data assigned in action):
$this->template = $this->createTemplate(ClassName);

Fyi, seems like you can't do this because template is marked as @property-read in https://github.com/nette/application/blob/d53d267f4c568bf848d2381b2c2aa28c580c1ed2/src/Application/UI/Control.php#L18 and Nette throws something about member access. Re-adding the property in a child class with just @property doesn't work for me. It would probably be my preferred "explicit" way.

Hmm than maybe just use local variable?

@spaze
Copy link
Owner Author

spaze commented Jan 25, 2023

Yeah, there are some ways how to do that. One of them is to overwrite then formatTemplateClass() method but all the ways have their pros and cons. Will have to choose one :-)

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.

2 participants