wiremock-php icon indicating copy to clipboard operation
wiremock-php copied to clipboard

Missing typhints for nearly everything

Open pscheit opened this issue 7 months ago • 6 comments

Hey,

thanks for porting this: Its really helpful having this, when using wiremock. I noticed that a lot classes (e.g. WireMock::create()) are missing docblocks/typehints.

Would you be willing to type everything correctly? Maybe even bump the PHP Version to 8.x?

I am happy to provide a pull request.

Best regards Philipp

pscheit avatar Jun 24 '25 05:06 pscheit

for example:

class WireMock
{
    /** @var string */
    private $hostname;
    /** @var int */
    private $port;
    /** @var HttpWait */
    private $httpWait;
    /** @var Curl  */
    private $curl;
    /** @var Serializer */
    private $serializer;

    public static function create($hostname = 'localhost', $port = 8080)
    {
        $httpWait = new HttpWait();
        $curl = new Curl();

        $serializer = SerializerFactory::default();
        return new self($httpWait, $curl, $serializer, $hostname, $port);
    }

becomes:

class WireMock
{
    private string $hostname;
    private int $port;
    private HttpWait $httpWait;
    private Curl $curl;
    private Serializer $serializer;

    public static function create(string $hostname = 'localhost', string $port = 8080): self
    {
        $httpWait = new HttpWait();
        $curl = new Curl();

        $serializer = SerializerFactory::default();
        return new self($httpWait, $curl, $serializer, $hostname, $port);
    }

pscheit avatar Jun 24 '25 05:06 pscheit

Hello! You're welcome for the port - it was just something I personally needed back in 2013, and I'm glad it's proven useful for others, too.

As you say, type declarations are almost entirely missing - because they weren't supported when I wrote it! And when they came in, I still wanted to support older PHP version for a while.

I'm pretty out of the loop with PHP, these days, but my understanding is:

  • Support for most type declarations comes in PHP 7.4 (https://www.php.net/manual/en/language.types.declarations.php)
  • PHP 7.4 has been out of security support since Nov 2022 (https://endoflife.date/php)
  • Most people are using at least PHP 8.1, but usage of PHP 7.4 is actually not insignificant; only about 50% of popular libraries have a minimum version of 8.0 (https://stitcher.io/blog/php-version-stats-january-2025)

Do you think you could submit a PR that bumps the minimum requirement to PHP 7.4 and replaces the @var comments with native type declarations? I'd be very happy to accept that.

I'm not sure what you have in mind when you mention docblocks, though?

rowanhill avatar Jun 24 '25 08:06 rowanhill

PHP 7.4 sounds good.

Especially that stitcher articles was discussed from Brent recently: its really amazing how authors are trying to be backwards compatible - but maybe shouldn't :)

https://www.youtube.com/watch?v=Z4b5gqKSZmA

But totally respect your decision there!

Let me cook something up. How is the test coverage? Can I rely on the fact, that if the tests run I wont break the most things? :D

pscheit avatar Jun 24 '25 12:06 pscheit

oh by docblock i just meant @param @var etc etc

Do you think you could submit a PR that bumps the minimum requirement to PHP 7.4 and replaces the @var comments with native type declarations? I'd be very happy to accept that.

I think in a first iteration I would type everything with @var , @param etc, and then in a second step could do this into native types.

pscheit avatar Jun 24 '25 14:06 pscheit

Unless you want to go full beast mode.. This is possible, too :D

pscheit avatar Jun 24 '25 14:06 pscheit

Aijaijai, this is more difficult than I thought.

It was way more code to fix, and could only fix src/Wiremock (excluding Serde and SerdeGen classes).

Took me a while to decipher what Serde is and does and while adding (and removing) lots of @var annotations - especially for arrays - I now succesfully broke how the DTOs are serialized from wiremock responses.

It does not even work with native types (only), because Serde expects docblocks to be present.

Is Serde something that you wrote? Or is this ported from wiremock as well?

Here is my current state: #24

Guess I need to leave the internal classes of the protocol as is and only fix the User facing API - that would already help with the usage in a typed project.

maybe - not sure

pscheit avatar Jun 24 '25 18:06 pscheit