Skip to content

Add phpstan and Resolve issues#666

Open
bhavz-10 wants to merge 9 commits intotheme-elementary-v2from
feature/issue-634-add-phpstan
Open

Add phpstan and Resolve issues#666
bhavz-10 wants to merge 9 commits intotheme-elementary-v2from
feature/issue-634-add-phpstan

Conversation

@bhavz-10
Copy link
Copy Markdown

@bhavz-10 bhavz-10 commented Apr 15, 2026

Description

Setup phpstan for the theme.

Technical Details

Adding phpstan setup along with the baseline setup so that errors that are to be ignored can be added and setup a baseline again.

Checklist

Screenshots

To-do

Fixes/Covers issue

Fixes #634

@bhavz-10 bhavz-10 changed the title feat: add phpstan and resolve issues Add phpstan and Resolve issues Apr 22, 2026
@bhavz-10 bhavz-10 marked this pull request as ready for review April 22, 2026 11:30
@bhavz-10 bhavz-10 self-assigned this Apr 22, 2026
@bhavz-10 bhavz-10 marked this pull request as draft April 22, 2026 11:30
@bhavz-10 bhavz-10 marked this pull request as ready for review April 22, 2026 11:44
Copy link
Copy Markdown

@Adi-ty Adi-ty left a comment

Choose a reason for hiding this comment

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

The issue mentions that level 5 is the target and that any pre-existing issues should be handled via a baseline rather than lowering the level. Moving to level 8 feels like a fairly meaningful step up in strictness, so I wanted to check whether that was intentional. If it was, could we call that out explicitly and get sign-off on the tighter standard? Otherwise, it may make sense to keep this at level 5 for now and rely on the baseline as described in the issue.

Comment thread inc/Framework/Traits/Singleton.php Outdated
* Collection of instance.
*
* @var array
* @var array<string, static>
Copy link
Copy Markdown

@Adi-ty Adi-ty Apr 23, 2026

Choose a reason for hiding this comment

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

array<string, static> works here, but the key type could be a bit more precise and the value type is slightly broader in practice because the cache can hold instances for multiple classes. Consider array<class-string, object> for the cache and narrowing the looked-up entry to static at return time with /** @var static $singleton */ before the return.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is right 🙌
I added the fix for this and pushed it in the PR

Comment thread phpstan-baseline.neon
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A small stub file wired via bootstrapFiles resolves it cleanly and lets us drop the baseline entry. Happy to leave it for now if you prefer to keep the change small.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added a boostrap file and removed the errors from baseline file and left it empty.

@bhavz-10
Copy link
Copy Markdown
Author

The issue mentions that level 5 is the target and that any pre-existing issues should be handled via a baseline rather than lowering the level. Moving to level 8 feels like a fairly meaningful step up in strictness, so I wanted to check whether that was intentional. If it was, could we call that out explicitly and get sign-off on the tighter standard? Otherwise, it may make sense to keep this at level 5 for now and rely on the baseline as described in the issue.

Hello @aryanjasala

Why we should take a stricter approach:

  • Adjustability: We can always reduce the strictness later if it causes a nuisance.
  • Customization: Users can still use the baseline for their own custom setups.
  • Visibility: Rather than decreasing the level to hide issues, we will maintain a baseline with documented errors. This ensures they are tracked and can be resolved at a later point.

* @param string $block_content Block content.
* @param array $block Block.
* @param string $block_content Block content.
* @param array<mixed> $block Block.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

$block parameter is typed as array<mixed> across all three methods, which allows integer keys, but the parsed block array always uses string keys (blockName, attrs etc.). array<string, mixed> would be a more accurate type here.

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