Skip to content

Conversation

@gauthierm
Copy link
Member

@gauthierm gauthierm commented Nov 19, 2025

Description

Adds PHPstan property declarations for module definitions.

Testing Instructions (optional)

Add step-by-step instructions for testing the PR, if necessary.

  1. Make sure CI passes

Developer Checklist

Before requesting review for this PR, make sure the following tasks are
complete:

  • I added a link to the relevant Shortcut story, if applicable
  • I added testing instructions, if any
  • I made sure existing CI checks pass
  • I checked that all requirements of the ticket are fulfilled

Reviewer Checklist

Before merging this PR, make sure the following tasks are complete:

  • I made sure there are no active labels that block merge
  • I followed the testing instructions
  • I made sure the CI checks pass
  • I reviewed the file changes on GitHub
  • I checked that all requirements of the ticket (if any) are fulfilled

*/
class StoreCacheTableUpdater extends SiteCommandLineApplication
{
public MDB2_Driver_Common $db;
Copy link
Contributor

Choose a reason for hiding this comment

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

Above, we set the type with a docblock comment. The worry I have with explicitly typing the property would be that we run into the "access before setting" issue ... in which case it needs to be nullable (and then we need to do null checks throughout the code).

Any concerns on your end?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had that concern as well. I did test the cache table updater and it runs correctly. I think if we ever had a access before setting issue in the code, we'd also have a null-dereference issue as none of the code checks for null on this property.

@cviebrock cviebrock merged commit 59cf871 into silverorange:master Nov 20, 2025
1 of 2 checks passed
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