Typed properties

[thomascorthals]

A new major version is the perfect opportunity to do some overhaul on the codebase. I've replaced the type hints with actual type declarations on class properties (introduced in PHP 7.4). And I've added union type declarations to method signatures (introduced in PHP 8.0) where this was previously also only done in type hints.

This is a breaking change for anyone extending our codebase, which I've added to the pitfalls. But it shouldn't break much for regular use of the library since I didn't have to change anything in the integration tests and they still pass.

[codecov[bot]]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 98.19%. Comparing base (80e85f5) to head (4d66547).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1168      +/-   ##
==========================================
+ Coverage   98.16%   98.19%   +0.02%     
==========================================
  Files         404      404              
  Lines       10632    10633       +1     
==========================================
+ Hits        10437    10441       +4     
+ Misses        195      192       -3     

Flag	Coverage Δ
unittests	98.19% <100.00%> (+0.02%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[mkalkbrenner]

@thomascorthals I agree, we should do that for 7.0.0. But what do you think? Shouldn't we declare interfaces for every class and use these interfaces as type properties?

[thomascorthals]

But what do you think? Shouldn't we declare interfaces for every class and use these interfaces as type properties?

Hadn't really thought about that. Would add additional work on our side to keep them in sync.

I was actually looking into getting the codebase to PHPStan level 2 and a lot of the errors seem to be caused by type hinting with interfaces that don't define all methods that are present in the implementing class. Here's an example, there are currently dozens like this.

 ------ ---------------------------------------------------------------------------------------------
  Line   Component\Facet\JsonFacetTrait.php (in context of class Solarium\Component\Facet\JsonTerms)
 ------ ---------------------------------------------------------------------------------------------
  140    Call to an undefined method Solarium\Component\Facet\FacetInterface::serialize().
         🪪  method.notFound
 ------ ---------------------------------------------------------------------------------------------

 ------ -----------------------------------------------------------------------------------
  Line   Component\RequestBuilder\FacetSet.php
 ------ -----------------------------------------------------------------------------------
  55     Call to an undefined method Solarium\Component\Facet\FacetInterface::getField().
         🪪  method.notFound
  65     Call to an undefined method Solarium\Component\Facet\FacetInterface::getField().
         🪪  method.notFound
  98     Call to an undefined method Solarium\Component\Facet\FacetInterface::serialize().
         🪪  method.notFound
 ------ -----------------------------------------------------------------------------------

[mkalkbrenner]

OK, then fix the existing interfaces and avoid further interfaces until required?

[thomascorthals]

I don't think we can "fix" the existing interfaces. I've done the PHPStan level 2 fixes in the code in my own branch because it's easier to just show what it takes: https://github.com/thomascorthals/solarium/commit/a4c09bc160104e2f4016f03e037a89d45f660a3d.

What can't be fixed with type declarations is usually fixable by type hinting. But that doesn't "fix" the interface.

Take the build() method for instance. It's signature in AbstractRequestBuilder asks for a QueryInterface.

    public function build(QueryInterface $query): Request

It can't be more specific than that because every class inheriting from it has a different Query class to build for. For Select it has this signature.

    public function build(QueryInterface|SelectQuery $query): Request

Because of contravariance, we can make the accepted parameters wider. That's what the union type does. However, static analysis doesn't "know" that we're only ever passing SelectQuery and reports all of its methods (setStart(), setRows()) as method.notFound. This isn't completely superfluous though as IDEs are able to provide autocompletion for $query->... with that function signature.

There is a workaround for static analysis if you use intersection types. Because that means narrowing the accepted type for the method, it's not possible to do so in a type declaration. But PHPStan prioritises PHPDoc type hints over the actual type declaration.

    /**
     * Build request for a select query.
     *
     * @param QueryInterface&SelectQuery $query
     *
     * @return Request
     */
    public function build(QueryInterface|SelectQuery $query): Request

This is the best solution I could come up with for the current structure of our codebase. It doesn't break backward compatibility, PHPStan knows what to do, IDEs know what to do, humans get the intent.

[thomascorthals]

@mkalkbrenner I think I have the bump to PHPStan level 2 ready to go in thomascorthals/solarium:phpstan. It's built off this branch but I prefer splitting it in different PRs. I think the current PR is stable as is.

[mpdude]

FWIW, I have tested these changes on a real-life project and it's all fine :+1: Have to admit though that the project does nothing fancy besides pushing content to the Solr index and querying 😁 .