phpstan / phpstan-nette

Nette Framework class reflection extension for PHPStan & framework-specific rules
MIT License
100 stars 35 forks source link

LinksRule for checking validity of links #132

Open MartinMystikJonas opened 1 year ago

MartinMystikJonas commented 1 year ago

Two questions: 1) Should this feature checkLinks be enabled by default? 2) I can add check for creating links to deprecated actions/signals (nette raises E_DEPRECATED in such cases) - should it be part of default behaviour or optional configuration checkDeprecatedLinks?

MartinMystikJonas commented 1 year ago

I wrote tests for all main use-cases but there is many ways how link creation can be done so it would require some real-world testing. I will try it on our real-world projects soon.

MartinMystikJonas commented 1 year ago

Noticed few issues when tested on real world apps. I will fix it soon.

MartinMystikJonas commented 1 year ago

All issues fixed and I added also checks for signals to sub-components. From my point of view it is finished. Only question is if I should add checks for deprecated links.

@ondrejmirtes Let me know what you think. Once finished I will write docs.

MartinMystikJonas commented 1 year ago

README added

MartinMystikJonas commented 1 year ago

Should be ready for final review

MartinMystikJonas commented 1 year ago

There is one more question. At this moment links to non-existing actions in presenters are ignored. Because presenters in Nette supports implicit actions without any action/render method. Existence or latte template on any of supported paths is enough for action to work. Problem is that paths where templates are searched could be sustopised by overriding formatTemplateFiles().

So I am not sure how to handle it. Always report links to implicit actions are possible errors (would create false-positive for implicit actions)? Check default paths for tempalte file and report if it does not exists there (woudl create false-positives for implciit action if formatTemplateFiles() is overriden)? Or implement some way how to customise this mechanism?

MartinMystikJonas commented 1 year ago

Implemented version: "Check default paths for tempalte file and report if it does not exists there (would create false-positives for implicit action if formatTemplateFiles() is overriden)". We can add way to specify custom template finding logic if someone would need it.

I also added option to support custom PresenterFactory without need to define custom PresenterREsolver service in PHPStan if it implements unformatPresenterClass (requested by @mabar)

And i added support for customised formatters of action/render/handle methods (also requested by @mabar)

See discussion on Slack https://pehapkari.slack.com/archives/C3K1ZA431/p1691600379411289

MartinMystikJonas commented 1 year ago

Fixed few edge-case errors reported by SamuelThorn (relative links in abstract presenters, use reflection not default class_exists prom PresenterFactory to check if presenter class exists, ...)

MartinMystikJonas commented 1 year ago

@ondrejmirtes Hi, do you have idea when you would have time to review this? If you have too much other work right now I think I can release this as standalone extension for now. But for compatibility reasons it would be nice to merge at lease containerLoader logic right into phpstan-nette as separate PR to avoid possible future conflicts, What do you think?

MartinMystikJonas commented 11 months ago

@ondrejmirtes I am really sorry for reminding you again. I know you have lots of other work to do. I would just like to know if you would have some time to review this PR? If not it is no problem, I will just made it standalone extension.

ondrejmirtes commented 11 months ago

Yeah, I'm sorry, I don't have the capacity to carefully review 1700 lines of code right now, please make it a new extension 😊

Ideally in the future both Latte and Links extensions would be served by phpstan-nette, but they have to work in all Nette applications without any asterisks.

MartinMystikJonas commented 11 months ago

Ok I will make it a separate extension. I understand it is a quite big PR so review would take lots of your valuable time.