Modernize code #155

BastianLedererIcinga · 2025-12-09T09:41:28Z

Add property, parameter and return types.
Replace return type self with static.
Some InvalidArgumentExceptions became obsolete by the new parameter types, unit tests have been adjusted to expect a TypeError instead.

src/Query.php

src/Relation/BelongsToMany.php

src/Behaviors.php

src/Compat/FilterProcessor.php

src/ResolvedExpression.php

src/Query.php

The base branch was changed.

…tructor implementation

… they are

Where an InvalidArgumentException was caused by a simple type mismatch, typed parameters will now casue a TypeError instead. Tests have been adjusted accordingly.

lippserd · 2026-01-14T16:19:58Z

src/Query.php

     * @return $this
     */
-    public function peekAhead($peekAhead = true)
+    public function peekAhead($peekAhead = true): static


Can this be bool $peekAhead?

I've checked out all usages, the only cases where this could end up not being a bool are in some controllers like this one:
https://github.com/Icinga/icingaweb2-module-x509/blob/81b7e5b6611e5a0c09bb19faca194583a79b462c/application/controllers/UsageController.php#L88
Where view->compact is passed, which could technically return null or some other type. The ActionController always initializes this property with a bool in the constructor and all reassignments also use bool values. When testing the affected controllers in the UI everything worked fine aswell, so I suppose we can type the parameter as bool.

lippserd · 2026-01-14T16:20:23Z

src/Query.php

    /**
     * Yield the query's results
     *
     * @return \Generator


You can simplify this to Generator.

lippserd · 2026-01-14T16:23:43Z

src/Relation.php

     * @return \Generator
     */
-    public function resolve()
+    public function resolve(): \Generator


Add use statement and simplify both PHPDoc and return type to Generator.

lippserd · 2026-01-14T16:25:11Z

src/Relation/BelongsToMany.php

    }

-    public function resolve()
+    public function resolve(): \Generator


Add use statement.

lippserd · 2026-01-14T16:28:14Z

src/UnionQuery.php

+     * @return ?Query[]
     */
-    public function getUnions()
+    public function getUnions(): ?array


This method does not return null, does it?

lippserd · 2026-01-14T16:30:06Z

tests/RelationTest.php

        $this->assertSame($targetClass, $relation->getTargetClass());
    }

-    public function testSetTargetClassThrowsInvalidArgumentExceptionIfNotString()


This test can be removed because we are no longer testing an implementation detail, but PHP itself.

lippserd · 2026-01-15T09:29:27Z

src/Exception/InvalidRelationException.php

-    /** @var Model The target model */
-    protected $model;
+    /** @var ?Model The target model */
+    protected ?Model $model;


I think it always makes sense to initialize nullable properties with null, as you do everywhere else. Even here, where the constructor sets the property.

lippserd · 2026-01-15T10:39:33Z

src/Exception/InvalidRelationException.php

     * @return Model
     */
-    public function getModel()
+    public function getModel(): Model


This method may return null.

Instead of using the type \Generator add a use statement so that docstrings and return types can be cleaned up.

Since the implementation will return at least an empty array null can be removed from the type.

Since model is nullable in the constructor the getModel() return type must allow null.

The removed test previously expected an InvalidArgumentException, which became obsolete with the addition of typed parameters so the test is no longer necessary.

All usages are compatible with the bool type, so the parameter is typed as bool and the cast is removed.

cla-bot bot added the cla/signed label Dec 9, 2025

BastianLedererIcinga changed the base branch from main to 8.4/8.5-compatibility December 9, 2025 13:28

BastianLedererIcinga marked this pull request as ready for review December 9, 2025 13:30

BastianLedererIcinga requested a review from Jan-Schuppik December 9, 2025 13:30

Jan-Schuppik reviewed Dec 9, 2025

View reviewed changes

src/Query.php Outdated Show resolved Hide resolved

Jan-Schuppik reviewed Dec 9, 2025

View reviewed changes

src/Query.php Show resolved Hide resolved

BastianLedererIcinga force-pushed the modernize-code branch from a48b99b to 7d98bbe Compare December 9, 2025 15:11

BastianLedererIcinga requested a review from Jan-Schuppik December 9, 2025 15:12

BastianLedererIcinga self-assigned this Dec 10, 2025

Jan-Schuppik previously approved these changes Dec 10, 2025

View reviewed changes

BastianLedererIcinga force-pushed the modernize-code branch from 7d98bbe to e194919 Compare December 10, 2025 14:47

sukhwinder33445 requested changes Dec 11, 2025

View reviewed changes

BastianLedererIcinga requested a review from sukhwinder33445 December 11, 2025 10:49

BastianLedererIcinga force-pushed the modernize-code branch from 87bf2cd to a83714b Compare December 11, 2025 14:23

sukhwinder33445 previously approved these changes Dec 11, 2025

View reviewed changes

BastianLedererIcinga force-pushed the 8.4/8.5-compatibility branch 3 times, most recently from 40c7f0b to be36f28 Compare December 18, 2025 14:17

Base automatically changed from 8.4/8.5-compatibility to main January 7, 2026 12:55

BastianLedererIcinga added 9 commits January 8, 2026 08:42

Add types for properties, parameters and functions

2c854a5

Replace return type self with static

d63cf91

Fix and remove some wrong return types and apply review suggestions

aab4fe6

Initialize Query::model with null

18720ff

Declare InvalidRelationException::model as nullable to match the cons…

87f819d

…tructor implementation

Remove type in Relation::setTargetClass to leave thrown Exceptions as…

13de1a9

… they are

Replace some InvalidArgumentExceptions

4e0f47c

Where an InvalidArgumentException was caused by a simple type mismatch, typed parameters will now casue a TypeError instead. Tests have been adjusted accordingly.

Revert unecessary type additions

e3218df

Apply review suggestions

d929c58

Optimize imports

1538b72

BastianLedererIcinga force-pushed the modernize-code branch from a83714b to 1538b72 Compare January 8, 2026 07:43

sukhwinder33445 previously approved these changes Jan 13, 2026

View reviewed changes

lippserd requested changes Jan 15, 2026

View reviewed changes

BastianLedererIcinga added 5 commits January 16, 2026 08:06

Clean up usages of the Generator type

1669ff8

Instead of using the type \Generator add a use statement so that docstrings and return types can be cleaned up.

Remove null from return type and docstring of UnionQuery::getUnions()

f34efec

Since the implementation will return at least an empty array null can be removed from the type.

Fix return type of InvalidRelationException::getModel()

6f31cfc

Since model is nullable in the constructor the getModel() return type must allow null.

Remove obsolete test

34dc2b2

The removed test previously expected an InvalidArgumentException, which became obsolete with the addition of typed parameters so the test is no longer necessary.

Add bool type to the $peekAhead parameter

6a3071e

All usages are compatible with the bool type, so the parameter is typed as bool and the cast is removed.

BastianLedererIcinga dismissed sukhwinder33445’s stale review via 6a3071e January 16, 2026 08:48

BastianLedererIcinga requested a review from lippserd January 16, 2026 08:49

Modernize code #155

Are you sure you want to change the base?

Modernize code #155

Uh oh!

Conversation

BastianLedererIcinga commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

BastianLedererIcinga commented Dec 9, 2025 •

edited

Loading