Skip to content

Handle ini_get and session_id failures for PHPStan level 7 - #1674

Merged
cpeel merged 2 commits into
DistributedProofreaders:masterfrom
bpfoley:level-7-get-ini
Oct 6, 2026
Merged

cpeel merged 2 commits into
DistributedProofreaders:masterfrom
bpfoley:level-7-get-ini

Conversation

@bpfoley

@bpfoley bpfoley commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

@cpeel cpeel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is one of those cases where PHPstan is being stupid.

Returns false if the configuration option doesn't exist.

But all of these are guaranteed to exist because we set them up on line 3.

I'd really rather tell PHPstan to stuff it than make the code more convoluted just to appease it.

@bpfoley

bpfoley commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator Author

This is one of those cases where PHPstan is being stupid.

Returns false if the configuration option doesn't exist.

But all of these are guaranteed to exist because we set them up on line 3.

I'd really rather tell PHPstan to stuff it than make the code more convoluted just to appease it.

So.... reading the (docs and comments for ini_set)[https://www.php.net/manual/en/function.ini-set.php], it turns out there are corner cases where ini_set can fail. PHPStan is certainly being annoyingly pedantic here, but it's not necessarily wrong.

I've tried using PHPDoc hints on most of the get_ini call sites to tell it we know what we're doing, but because you can't do inline type hints, you still need to assign to variables and the cure is almost as bad as the sickness.

The more terse solution would be just to cast to string/int. Technically that's a behavioural change, but we can go with that if you like.

Let me know what you think.

@cpeel

cpeel commented Oct 5, 2026

Copy link
Copy Markdown
Member

What if we rethink this just a bit and use either file globals (namespaced globals?) or some other simple data structure to store these session config values. Then use that to ini_set() the values. Then we don't need to ini_get() anything, we can just use the values we already set. That's probably a bit cleaner for what we're trying to achieve here.

@cpeel cpeel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Brilliant.

@cpeel
cpeel merged commit ed1fb10 into DistributedProofreaders:master Oct 6, 2026
12 checks passed
@bpfoley
bpfoley deleted the level-7-get-ini branch October 6, 2026 23:25
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