Improved typing

[SamMousa]

This PR improves typing and fixes issues identified by phpstan.
It also removes some functions that were already marked as @internal (so it doesn't count as breaking backward compatibility)

[boboldehampsink]

This is breaking the implementation in Craft CMS's test class, as the Yii2 Module now is a "final" class? This is not really BC-compatible. If intended, it should atleast be a new version?

[SamMousa]

You are right, changing classes to final should have been a major version update.

Are you up for making a PR that reverts the final change? Note that we will create a new major release where these classes will be final.

Extending 3rd party classes is generally not the way to go as your code becomes very strongly coupled to the library.

[samdark]

Un-finalized the module in master. Anything else?

[samdark]

Tagged release.

[boboldehampsink]

Tagging @brandonkelly, which has code that extends this module: https://github.com/craftcms/cms/blob/5.x/src/test/Craft.php

[boboldehampsink]

Un-finalized the module in master. Anything else?

I haven't tested it any further, will do tomorrow.

[boboldehampsink]

@brandonkelly the latest release of this module results in the following:

PHP Fatal error:  Declaration of craft\test\CraftConnector::startApp(?yii\log\Logger $logger = null) must be compatible with Codeception\Lib\Connector\Yii2::startApp(?yii\log\Logger $logger = null): void in /app/user/vendor/craftcms/cms/src/test/CraftConnector.php on line 135

Types need to be updated accordingly in Craft as well

[SamMousa]

Is the inheritance really needed? Why not use composition?

…

On Tue, 10 Dec 2024, 08:04 Bob Olde Hampsink, ***@***.***> wrote: @brandonkelly <https://github.com/brandonkelly> the latest release of this module results in the following: PHP Fatal error: Declaration of craft\test\CraftConnector::startApp(?yii\log\Logger $logger = null) must be compatible with Codeception\Lib\Connector\Yii2::startApp(?yii\log\Logger $logger = null): void in /app/user/vendor/craftcms/cms/src/test/CraftConnector.php on line 135 Types need to be updated accordingly in Craft as well — Reply to this email directly, view it on GitHub <#96 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEFRTNSWOBB5EHZLHJFQXT2E2G7ZAVCNFSM6AAAAABTIENMWSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMZQGYYDOMBUHA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[boboldehampsink]

This would be something for the Craft team to consider. To get some insight, here is how they are using this module: https://github.com/craftcms/cms/tree/5.x/src/test

By the way, I think even changing typing like this is worth a new major release, as its still not backward compatible.

[brandonkelly]

@boboldehampsink We updated the return type for CraftConnector::startApp() in Craft 5.5.6.

[SamMousa]

This would be something for the Craft team to consider. To get some insight, here is how they are using this module: https://github.com/craftcms/cms/tree/5.x/src/test

By the way, I think even changing typing like this is worth a new major release, as its still not backward compatible.

I agree, it was a mistake by me, which is why we reverted. Itll be in a major version bump.

[boboldehampsink]

@boboldehampsink We updated the return type for CraftConnector::startApp() in Craft 5.5.6.

Thanks! Any chance of a backport for v4 as well?

[brandonkelly]

@boboldehampsink We didn’t override that method until Craft 5. And Craft 4 is stuck on codeception/module-yii2 1.1.5, as 1.1.6 is when they started requiring Codeception 5.

If you’re using Codeception 5 with Craft 4, please PR whatever changes you need to Craft. Assuming they don’t break our own tests, we can pull them in.

[boboldehampsink]

@brandonkelly we're using Codeception 5 with Craft 4 indeed. That would just be the same fix, correct?

[brandonkelly]

As I said, we weren’t overriding that method until Craft 5.