Skip to content

Add strict type declarations and modernize codebase#96

Open
sukhwinder33445 wants to merge 5 commits intomainfrom
modernize-code
Open

Add strict type declarations and modernize codebase#96
sukhwinder33445 wants to merge 5 commits intomainfrom
modernize-code

Conversation

@sukhwinder33445
Copy link
Contributor

@sukhwinder33445 sukhwinder33445 commented Dec 9, 2025

Add strict type declarations to all method parameters, return types, and
properties throughout the codebase. Remove redundant runtime type checks
now enforced by the type system. Update PHPDoc to use nullable shorthand
and compact format. Replace list() with array destructuring, and
switch/case with match expressions. Remove the InAndNotInUtils
trait, which was introduced to suppress PHPStan errors before strict
typing was available. Clean up deprecated test helpers.

Raise the minimum required ipl/stdlib version to adopt its strict type
declarations.

requires Icinga/ipl-stdlib#63

@cla-bot cla-bot bot added the cla/signed label Dec 9, 2025
@sukhwinder33445 sukhwinder33445 self-assigned this Dec 9, 2025
@sukhwinder33445 sukhwinder33445 changed the base branch from main to support-php-8.4/8.5 December 9, 2025 08:19
@sukhwinder33445 sukhwinder33445 force-pushed the modernize-code branch 2 times, most recently from eefa5b5 to 8686b59 Compare December 9, 2025 10:51
@BastianLedererIcinga
Copy link
Contributor

The InAndNotUtils trait is currently incompatible with ipl\Stdlib\Filter\Condition.

@sukhwinder33445 sukhwinder33445 force-pushed the modernize-code branch 3 times, most recently from 589584f to c0df0ec Compare December 11, 2025 08:00
@BastianLedererIcinga
Copy link
Contributor

The rest looks good.

@BastianLedererIcinga BastianLedererIcinga force-pushed the support-php-8.4/8.5 branch 2 times, most recently from 2ebbbb8 to 9aaf2fd Compare December 18, 2025 15:01
Base automatically changed from support-php-8.4/8.5 to main January 7, 2026 13:00
@lippserd lippserd dismissed BastianLedererIcinga’s stale review January 7, 2026 13:00

The base branch was changed.

Copy link
Contributor

@BastianLedererIcinga BastianLedererIcinga left a comment

Choose a reason for hiding this comment

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

Local unit tests all pass when using the other modernize branches.

Copy link
Member

@lippserd lippserd left a comment

Choose a reason for hiding this comment

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

requires Icinga/ipl-i18n#26 (for tests to run, compatibility issue)

Why? ipl-i18n is not a dependency of either ipl-sql or ipl-stdlib.


77124d2 states "non-breaking", but it does introduce strict types on Cursor::limit() and Cursor::offset(), which requires a newer version of ipl-stdlib. Plus, the changes only become testable through follow-up commits, which spreads related changes across multiple places and makes it harder to reason about the impact.

To keep things manageable especially with all the releases coming soon, it helps if we follow a few rules:

When we say non-breaking, we mean it: no dependency or consumer must need to change. Any test adjustments belong in the same commit, and if a newer PHP version is needed, composer.json should reflect that in the same commit with a note in the description.

When a change in an IPL component does require updates in consumers,
those consumer updates should land in a single commit that also bumps the version constraint in composer.json.
Since we don't have releases yet, pointing to a branch is totally fine:
ipl-i18n#13 is a good example of how to do that.
We'll change those to proper version constraints once we actually release.

In the end, we're aiming for two distinct release steps:

  • first a non-breaking IPL release that's safe for Icinga Web and its modules,
  • then a follow-up breaking release that coordinates updates across everything affected.

Clean, self-contained commits are important.

One more thing worth clarifying: breaking changes that only affect IPL don't need to live in a separate PR as long as the commits are self-contained.
However, anything that breaks Icinga Web and/or its modules does need to go into a new PR.

@BastianLedererIcinga @Jan-Schuppik Please check your PRs to make sure they comply with these rules. I've already adjusted Icinga/ipl-stdlib#63.

@sukhwinder33445 sukhwinder33445 force-pushed the modernize-code branch 2 times, most recently from 8bafaff to b244e12 Compare March 16, 2026 11:43
@sukhwinder33445
Copy link
Contributor Author

I have adjusted the commits accordingly. Each commit is now self-contained.

Copy link
Member

@lippserd lippserd left a comment

Choose a reason for hiding this comment

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

Please squash 197e119 and e752d91 into b244e12 as all changes are in response to ipl-stdlib introducing strict typing. Change commit title to, e.g., "Adapt to ipl-stdlib strict typing" and mention all changes in the commit description.

sukhwinder33445 and others added 4 commits March 19, 2026 14:15
Add strict type declarations to properties, function/method signatures, where
types are unambiguous and no inheritance or interface contract is affected.

Align tests with strict typing and tighten PHPDoc where applicable.

Co-authored-by: Eric Lippmann <eric.lippmann@icinga.com>
`setupTest()` had an empty body, so no replacement is necessary.
`ipl-stdlib` introduces strict type declarations, requiring all classes
implementing its interfaces to match the exact parameter and return types
defined there. Updated affected classes and bumped the required `ipl-stdlib`
version in `composer.json` accordingly.

Remove the now obsolete `@throws InvalidArgumentException` annotation since the
strict `arrayval()` function no longer throws this exception.

Remove the now redundant `InAndNotInUtils` trait, which was originally added to
suppress PHPStan errors before strict typing was introduced.
@lippserd lippserd changed the title Modernize code Add strict type declarations and modernize codebase Mar 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants