Modernise code by mrliptontea · Pull Request #156 · talis/tripod-php

mrliptontea · 2026-02-27T15:01:45Z

Precursor to PLT-951
- Modernise codebase with PHPStan, PHP CS Fixer, and Rector
- Drop PHP 7.3
- Use PHP 7.4 features
- Strict types where possible
- Resolve todo: use ObjectId->getTimestamp()

Used rector.php

<?php

declare(strict_types=1);

use Rector\CodeQuality\Rector\Empty_\SimplifyEmptyCheckOnEmptyArrayRector;
use Rector\CodeQuality\Rector\Identical\FlipTypeControlToUseExclusiveTypeRector;
use Rector\CodingStyle\Rector\Catch_\CatchExceptionNameMatchingTypeRector;
use Rector\CodingStyle\Rector\ClassMethod\MakeInheritedMethodVisibilitySameAsParentRector;
use Rector\CodingStyle\Rector\PostInc\PostIncDecToPreIncDecRector;
use Rector\Config\RectorConfig;
use Rector\Naming\Rector\Assign\RenameVariableToMatchMethodCallReturnTypeRector;
use Rector\Naming\Rector\Class_\RenamePropertyToMatchTypeRector;
use Rector\Naming\Rector\ClassMethod\RenameParamToMatchTypeRector;
use Rector\Naming\Rector\ClassMethod\RenameVariableToMatchNewTypeRector;
use Rector\Naming\Rector\Foreach_\RenameForeachValueVariableToMatchExprVariableRector;
use Rector\Privatization\Rector\Class_\FinalizeTestCaseClassRector;
use Rector\Strict\Rector\Empty_\DisallowedEmptyRuleFixerRector;
use Rector\TypeDeclaration\Rector\ClassMethod\ParamTypeByMethodCallTypeRector;
use Rector\TypeDeclarationDocblocks\Rector\Class_\AddReturnDocblockDataProviderRector;
use Rector\TypeDeclarationDocblocks\Rector\ClassMethod\AddParamArrayDocblockFromDimFetchAccessRector;
use Rector\TypeDeclarationDocblocks\Rector\ClassMethod\DocblockReturnArrayFromDirectArrayInstanceRector;

return RectorConfig::configure()
    ->withPaths([
        __DIR__ . '/scripts',
        __DIR__ . '/src',
        __DIR__ . '/test',
    ])
    ->withBootstrapFiles([
        __DIR__ . '/src/tripod.inc.php',
    ])
    ->withPhpSets(php74: true)
    ->withPreparedSets(
        deadCode: true,
        codeQuality: true,
        codingStyle: true,
        typeDeclarations: true,
        typeDeclarationDocblocks: true,
        privatization: true,
        naming: true,
        instanceOf: true,
        earlyReturn: true,
        rectorPreset: true,
        phpunitCodeQuality: true,
    )
    ->withSkip([
        AddReturnDocblockDataProviderRector::class,
        CatchExceptionNameMatchingTypeRector::class,
        DisallowedEmptyRuleFixerRector::class,
        FinalizeTestCaseClassRector::class,
        MakeInheritedMethodVisibilitySameAsParentRector::class,
        ParamTypeByMethodCallTypeRector::class,
        PostIncDecToPreIncDecRector::class,
        RenameForeachValueVariableToMatchExprVariableRector::class,
        RenameParamToMatchTypeRector::class,
        RenamePropertyToMatchTypeRector::class,
        RenameVariableToMatchMethodCallReturnTypeRector::class,
        RenameVariableToMatchNewTypeRector::class,
        SimplifyEmptyCheckOnEmptyArrayRector::class,
        FlipTypeControlToUseExclusiveTypeRector::class,
        AddParamArrayDocblockFromDimFetchAccessRector::class,
        DocblockReturnArrayFromDirectArrayInstanceRector::class,
    ])
;

astilla · 2026-03-20T09:19:25Z

src/classes/ChangeSet.php

                    $a[$rdf] = $parser->getSimpleIndex(0);
                } elseif (
-                    is_array($a[$rdf])
-                    and isset($a[$rdf][0])


I'd long since forgotten and existed in PHP...!

astilla · 2026-03-20T09:25:27Z

src/classes/ExtendedGraph.php

+     * @param ObjectType      $type
     */
-    private function get_subjects_where($p, $o, $type)
+    private function get_subjects_where(string $p, $o, string $type): array


Why does the docblock type it as a TriplePredicate but in the function it types it as a string ?

Types like TriplePredicate, ObjectValue are PHPStan type aliases, defined here:

tripod-php/src/classes/ExtendedGraph.php

Lines 12 to 17 in 2adc875

* @phpstan-type ObjectResource string

* @phpstan-type ObjectLiteral string|int|float|bool

* @phpstan-type ObjectType 'bnode'|'uri'|'literal'

* @phpstan-type ObjectValue ObjectResource|ObjectLiteral

* @phpstan-type TripleSubject string

* @phpstan-type TriplePredicate string

(though language servers in PHPStorm, Intelephense in VSCode, etc. also interpret them and provide completions based on them)

I'm using TriplePredicate for consistency, even though it translates to just string. This helps make a better sense of what's inside the object, which is especially visible here:

tripod-php/src/classes/ExtendedGraph.php

Line 19 in 2adc875

* @phpstan-type TripleGraph array<TripleSubject, array<TriplePredicate, TripleObject[]>>

I think this enhances the reading experience as you can quickly find where predicates are referenced, such as here:

tripod-php/src/classes/ExtendedGraph.php

Lines 954 to 956 in 2adc875

* @return TriplePredicate[] list of property URIs

*/

public function get_subject_properties(string $s, bool $distinct = true): array

So you know it's not just an array being returned, it's an array of predicates.

astilla · 2026-03-20T09:25:58Z

src/classes/ExtendedGraph.php

+     * @param 'literal'|'resource' $type
     */
-    private function add_to_sequence($s, $o, $type = 'resource')
+    private function add_to_sequence(string $s, $o, string $type = 'resource'): void


Same question as above, but TripleSubject vs. string

astilla · 2026-03-20T09:44:15Z

phpstan.neon

@@ -0,0 +1,17 @@
+parameters:
+  level: 5


astilla

Well...it's massive... 🤣
Looks like some good automated updates.
I've left a couple of questions I wasn't sure about a difference in typing between docblock and function param definition.
Other than than it looks sane but being so big I've somewhat skimmed it.

mike-dean-talis

I reached the end. Holy moly. That's 2 hours I won't get back.

This generally looks good to me although I have a few questions and observations along the way.

mike-dean-talis · 2026-03-24T13:00:54Z

scripts/mongo/createSearchDocuments.php


-if (empty($options) || isset($options['h']) || isset($options['help'])
+if (
+    $options === [] || $options === false || isset($options['h']) || isset($options['help'])


comment: I am surprised that $options === [] works as intended to be honest. empty does check for false-y values, so I am surprised this is necessary.

This is on Rector: https://getrector.com/rule-detail/disallowed-empty-rule-fixer-rector.

Since PHP's getopt can return array|false that's what's it checking for.

From type safety standpoint this make more sense, since the definition of what "empty" means in PHP is quite loose...

$ php -r 'var_dump(empty("0"));' bool(true)

mike-dean-talis · 2026-03-24T13:06:06Z

scripts/mongo/createSearchDocuments.php

-} else {
-    $id = null;
-}
+$id = isset($options['i']) || isset($options['id']) ? $options['i'] ?? $options['id'] : null;


comment: I wonder if we can make it simpler than this:

$id = $options['i'] ?? $options['id'] ?? null;

We seem to do similar elsewhere, so I won't go through them all (I'll be here forever) but you can get my point.

True, I missed this. Automagic tools can only take us so far I guess...

See bfc8602

mike-dean-talis · 2026-03-24T13:07:55Z

scripts/mongo/createTables.php

        Config::getInstance()->setMongoCursorTimeout(-1);

-        echo "Generating {$tableId}";
+        echo 'Generating ' . $tableId;


question: I am not entirely sure what was "bad" about the previous syntax? We do a lot of interleaved strings like this in JS, I just wonder what makes it "modern" to go backwards? Anyway... enough of my opinionated waffle.

Again, Rector at play: https://getrector.com/rule-detail/encapsed-strings-to-sprintf-rector

I believe it's about consistency:

In PHP, unlike JS, {} is limited, and only works with variables:

echo "{$var}"; echo "{$obj->prop}"; echo "{$obj->method()}"; echo "{$obj::$staticProp}"; echo "{$obj::static()}"; # But you can't e.g.: echo "{func()}"; echo "{CONSTANT}"; echo "{SomeClass::static()}";

There is also cursed, deprecated, "${var}" syntax.

mike-dean-talis · 2026-03-24T13:11:34Z

scripts/mongo/detectNamespaces.php

-$json = json_encode(['namespaces' => $config['namespaces']]);
-
-echo indent($json);
+echo json_encode(['namespaces' => $config['namespaces']], JSON_PRETTY_PRINT);


praise: This one made me laugh out loud. Why have 50 lines when you can have one? 👍

mike-dean-talis · 2026-03-24T13:17:35Z

scripts/mongo/loadTriples.php

    $subject = trim($parts[0], '><');

-    if (empty($currentSubject)) { // set for first iteration
+    if ($currentSubject === '' || $currentSubject === '0') { // set for first iteration


comment: This PR makes me wonder if there is something inherently wrong with empty? As it seems to be avoided in favour of direct comparisons that I wouldn't usually use.

See my comment above; but yes, Rector seem to prefer to avoid empty since it's a catch-all for null, false, '', '0', 0, 0.0, []. Converting to an explicit comparison forces you to think about types and possible values.

That said: 119a88f

mike-dean-talis · 2026-03-24T13:50:56Z

src/classes/Labeller.php

+            if (
+                preg_match('~^[a-zA-Z][a-zA-Z0-9\-]+$~', $parts[$i])
+                && !isset($this->_ns[$parts[$i]])
+                && $parts[$i] != 'schema'
+                && $parts[$i] != 'ontology'
+                && $parts[$i] != 'vocab'
+                && $parts[$i] != 'terms'
+                && $parts[$i] != 'ns'
+                && $parts[$i] != 'core'
+                && strlen($parts[$i]) > 3
+            ) {


comment: This is my favourite if statement. I have no idea what it's really doing or why.

No change necessary just... man. I despair.

We'll never know. It dates back to the Initial commit
https://github.com/talis/tripod-php/blame/428acd7e190e2427a8ee8dce724afc79b0ea249f/src/classes/Labeller.class.php#L345

mike-dean-talis · 2026-03-24T14:22:55Z

src/mongo/serializers/NQuadSerializer.php

+     * @param array $index
     */
-    public function getSerializedIndex($index, $context)
+    public function getSerializedIndex($index, ?string $context): string


question: Can we not type $index as an array? Maybe I have missed something obvious.

It's not you, it'm me (and Rector) who missed this.

Yes, we can type it: 4a8479e

mike-dean-talis · 2026-03-24T14:39:42Z

src/mongo/Driver.php

+        $milliseconds = (int) $lastUpdatedDate->__toString();
+        $seconds = intdiv($milliseconds, 1000);
+        $fraction = ($milliseconds % 1000) / 1000;
+
+        return sprintf('%.8f %d', $fraction, $seconds);


praise: I think this is doing the same work. I'm hoping tests would catch it if not. This makes the process more readable at least.

tripod-php/test/unit/mongo/MongoTripodDriverTest.php

Line 2278 in 477f576

$this->assertMatchesRegularExpression('/^0.[0-9]{8} [0-9]{10}/', $tripod->getETag($_id['r']));

Though why microtime format was chosen in the first place evades me...

mike-dean-talis · 2026-03-24T14:44:55Z

test/unit/mongo/ConfigGeneratorTest.php

        $this->assertInstanceOf(Tripod\Mongo\Config::class, $instance);
        $this->assertInstanceOf(ITripodConfigSerializer::class, $instance);
-        $this->assertEquals(
+        $this->assertSame(


praise: Bless your soul. assertSame.

mike-dean-talis · 2026-03-24T14:47:00Z

test/unit/mongo/EnsureIndexesTest.php

+    public function mandatoryArgDataProvider(): iterable
    {
-        return [
-            ['tripodConfig', 'tripodConfig or tripodConfigGenerator'],
-            ['storeName'],
-            ['reindex'],
-            ['background'],
-        ];
+        yield ['tripodConfig', 'tripodConfig or tripodConfigGenerator'];
+        yield ['storeName'];
+        yield ['reindex'];
+        yield ['background'];


praise: Another thing I didn't know PHP had... iterators. Well, I hope to add that one to my repertoire.

Generators are pretty neat.

But for writing tests with many cases wait for PHP attributes!
https://github.com/talis/depot/blob/b2d73c12/server/tests/unit/VaultTest.php#L19-L25

mrliptontea force-pushed the PLT-951-modernise-code branch 9 times, most recently from a1fa945 to 6a0ca80 Compare March 5, 2026 23:25

mrliptontea force-pushed the PLT-951-modernise-code branch 3 times, most recently from 30f20fc to 7d9faf7 Compare March 9, 2026 14:43

mrliptontea added 17 commits March 10, 2026 12:00

Apply Rector

8ff1fa2

Stricter typing in ExtendedGraph

d8f15e6

Use ObjectId->getTimestamp()

6f2502d

Fix call expectation

bde407b

Fixes after applying Rector

bb04e72

Fix Config::getReplicaSetName test & validate data source

3501f5d

Make lowercase modifier work with Stringable values

f46986c

Re-throw exceptions instead of wrapping them in another

66fbe41

Stub sleep to speed up retry tests

4107d39

Make assertDocumentDoesNotHaveProperty pass if document doesn't exist

00649d5

Types in MongoTripodTestBase

6b68f09

Fix types in MongoTransactionLogTest

ae320fe

Apply php-cs-fixer

13752a2

Format tests

22c5e0f

Format scripts

727c243

Format src

74ef976

Tolerate null values in StatsD

982da60

mrliptontea force-pushed the PLT-951-modernise-code branch from 7d9faf7 to 3cca01b Compare March 10, 2026 12:03

mrliptontea added 3 commits March 17, 2026 09:49

Labeller corrections

3c0a040

Fix typo

148d0db

Fix missing return types

18afec6

mrliptontea force-pushed the PLT-951-modernise-code branch from a98ede3 to 18afec6 Compare March 17, 2026 09:49

mrliptontea added 4 commits March 17, 2026 12:43

Allow null offset in Driver::select()

b25f9ad

Make Driver::getTableRows() more consistent with select()

af47f6e

Correct types

747e6d3

Ensure no empty values in newCBDs

86ed2a3

mrliptontea force-pushed the PLT-951-modernise-code branch from c912d90 to 2c80ef9 Compare March 17, 2026 13:06

Remove unneeded require

7ff25bc

mrliptontea force-pushed the PLT-951-modernise-code branch from 2c80ef9 to edac916 Compare March 17, 2026 13:09

mrliptontea added 2 commits March 17, 2026 14:26

Check storeName is set before accessing

d1c6901

Segregate methods from DriverBase

143e27b

mrliptontea force-pushed the PLT-951-modernise-code branch 2 times, most recently from ba3d8fe to 143e27b Compare March 18, 2026 11:20

mrliptontea marked this pull request as ready for review March 18, 2026 11:30

Fix predicate (key) validation in MongoGraph::add_tarray_to_index

2adc875

mrliptontea force-pushed the PLT-951-modernise-code branch from b76f569 to 2adc875 Compare March 19, 2026 13:25

astilla reviewed Mar 20, 2026

View reviewed changes

phpstan.neon

@@ -0,0 +1,17 @@

parameters:

level: 5

Copy link
Copy Markdown

Member

astilla Mar 20, 2026

Choose a reason for hiding this comment

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

Woop woop

mike-dean-talis reacted with hooray emoji

astilla reviewed Mar 20, 2026

View reviewed changes

Don't export phpstan files

351eb38

mike-dean-talis reviewed Mar 24, 2026

View reviewed changes

mrliptontea added 5 commits March 24, 2026 22:03

Simplify option checks in scripts

bfc8602

Correct usage text in scripts

3ddf74b

Fix check for initial $currentSubject

119a88f

Stronger types in NQuadSerializer

4a8479e

Add missing types

0f19674

	* @phpstan-type ObjectResource string
	* @phpstan-type ObjectLiteral string\|int\|float\|bool
	* @phpstan-type ObjectType 'bnode'\|'uri'\|'literal'
	* @phpstan-type ObjectValue ObjectResource\|ObjectLiteral
	* @phpstan-type TripleSubject string
	* @phpstan-type TriplePredicate string

	* @return TriplePredicate[] list of property URIs
	*/
	public function get_subject_properties(string $s, bool $distinct = true): array

Conversation

mrliptontea commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

astilla left a comment

Choose a reason for hiding this comment

Uh oh!

mike-dean-talis left a comment

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!

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!

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!

3 participants

mrliptontea commented Feb 27, 2026 •

edited

Loading