Mark classes as final or abstract when possible

[stloyd]

Pull Request

Related issue

Fixes n/a

What does this PR do?

Mark classes as final or abstract when possible, as part of the road for hard BC break ( #518) we can do less hard one with phpdoc declarations.

PR checklist

Please check if your PR fulfills the following requirements:

• Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
• Have you read the contributing guidelines?
• Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

[ppshobi]

this is a library package, also a meta package since a lot of packages extend from it. making classes final brings a lot of pain for the downstream libraries and even for us to do testing. what are the advantages of making classes final in our case?

[norkunas]

this is a library package, also a meta package since a lot of packages extend from it. making classes final brings a lot of pain for the downstream libraries and even for us to do testing. what are the advantages of making classes final in our case?

please give some examples where this brings pain to you

[ppshobi]

Just to elaborate my point

1. example: Final classes makes it harder to create mock. hence harder to test -> discourage people from using the library/contributing.

final class DocumentQuery {
}

final class HandlesDocumentQuery {
    public function post (DocumentQuery $documentQuery) {

    }
}

final class DocumentResult {

}

class TestFinalClass {
    public function testICanHandleDocumentQuery () {
        $documentQueryMock = $this->createMock(DocumentQuery::class);
        $documentResultMock = $this->createMock(DocumentResult::class)
        $this->createMock(HandlesDocumentQuery::class)
            ->expects(static::once())
            ->method('post')
            ->with($documentQueryMock)
            ->willReturn($documentResultMock); 

       // None of the above test works because you can't mock any of it, you will have to implement interfaces of each class

    }
}

3. removes the flexibility of extension of classes, why would you remove a flexibility when you(I mea meilisearch/meilisearch-php) have no real advantages? some things like exceptions are totally okay to be final (guess what! its made final by the php itself).
4. I think how a class (and its methods) should be used is on the shoulders of the developer and not on the library. do we want to force others learn our way of coding or let them be?
5. some more notes here: https://stackoverflow.com/questions/4248021/when-to-use-final-in-php , some discussion here https://www.reddit.com/r/PHP/comments/ajdgx6/when_to_declare_classes_final
6. some are even making packages to mock final classes (?) : https://github.com/nunomaduro/mock-final-classes

[norkunas]

DocumentQuery is a Value Object, what's the point of mocking it?

Also if you really need to mock something, I'd suggest to mock http client responses, not the meilisearch library parts

[stloyd]

1. Sorry but this is not a problem of SDK, we can and probably will provide an interface for crucial parts of the code but that's not an argument for not making the code final,
2. As above, you want to mock what you do not own, and you should never do that,
3. That's the point, it's SDK, it's based on an external tool to work and maintainers need to take care of features and bugs against official code, if you are allowed to extend the code it means that any bug is not a problem of the maintainer but yours, yet you will first ask the maintainer for support on your edge case, this removes that problem from the shoulder of maintainers,
4. Yes, we want developers to learn how to better software, that's why we should enforce good practices everywhere,
5. & 6. I will not comment on those two, it's pointless to comment on hacks or bad designs, there is no good use case to remove the final from class that is set as such.

Long story short: don't mock what you don't own.

The problem - that became visible thanks to final - is mocking
https://twitter.com/brendt_gd/status/1666773176901160962

[ppshobi]

I will try to justify a little. Does not want to take it as a final/non-final argument chain.

Final classes remove the flexibility of extending and modifying the behavior of classes. This can be limiting for developers who have legitimate use cases where they need to extend or customize certain classes. (Which I have explained as a sample in my earlier reply).

Why should I mock a class? its should be upto me, maybe i don't want to initialize all the parameters, maybe I like the flexibility of $this->createMock() or a million other reasons.

Assuming that the library's way of coding is the correct or desired way may not align with developers' preferences or needs. Allowing more flexibility in class extension empowers developers to make their own decisions and find the best solutions for their use cases.

we may require the ability to mock final classes. This could be due to limitations or dependencies in their testing frameworks or specific testing needs. By marking classes as final, the library imposes restrictions that may conflict with these requirements and make it harder for developers to test their code effectively. When a library is less flexible and less accommodating to various use cases, it may limit its adoption within the community. Allowing for more flexibility can attract a wider range of developers and increase the library's overall usage and contributions.

In the end keeping a balance between control and flexibility is important. I don't think allowing code extension doesn't mean losing control over the SDK; rather, it enables developers to extend and modify depending on their use case.

Also interesting to see what are the benefits of making these classes final!

[norkunas]

library is not responsible that you want to mock everything even VO's that doesn't make sense, but that's why I suggested to add just phpdoc with final annotations instead of making them really final, so we could see what could be reverted later.

and I suggest for you to write an integration test instead for the search because with mocks you don't ensure that it really works ❗