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

LinksRule for checking validity of links #132

Open
wants to merge 1 commit into
base: 1.1.x
Choose a base branch
from

Conversation

MartinMystikJonas
Copy link
Contributor

@MartinMystikJonas MartinMystikJonas commented Aug 8, 2023

No description provided.

@MartinMystikJonas MartinMystikJonas changed the title LinksRule LinksRule for checking validity of links Aug 9, 2023
@MartinMystikJonas MartinMystikJonas marked this pull request as ready for review August 9, 2023 16:51
@MartinMystikJonas
Copy link
Contributor Author

MartinMystikJonas commented Aug 9, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

MartinMystikJonas commented Aug 9, 2023

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

@MartinMystikJonas
Copy link
Contributor Author

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.

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document this feature in the README :) I'll do a thorough review later.

extension.neon Outdated Show resolved Hide resolved
@MartinMystikJonas
Copy link
Contributor Author

README added

@MartinMystikJonas
Copy link
Contributor Author

Should be ready for final review

@MartinMystikJonas
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Contributor Author

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.

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