Declaring methods final in classes

classic Classic list List threaded Threaded
24 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Declaring methods final in classes

Aryeh Gregor-4
I see that in some classes, like WANObjectCache, most methods are declared
final. Why is this? Is it an attempt to optimize?

The problem is that PHPUnit mocks can't touch final methods. Any ->method()
calls that try to do anything to them silently do nothing. This makes
writing tests harder.

If we really want these methods to be marked final for some reason, the
workaround for PHP is to make an interface that has all the desired
methods, have the class implement the interface, and make type hints all
refer to the interface instead of the class. But if there's no good reason
to declare the methods final to begin with, it's simplest to just drop it.
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Declaring methods final in classes

Daimona
Personally, I don't like these limitations in PHPUnit and the like. IMHO,
they should never be a reason for changing good code. And sometimes,
methods have to be final. I don't know about the specific case, though.

Anyway, some time ago I came across [1], which allows mocking final methods
and classes. IIRC, it does that by removing the `final` keywords from the
tokenized PHP code. I don't know how well it works, nor if it could degrade
performance, but if it doesn't we could bring it in via composer.


[1] - https://github.com/dg/bypass-finals


Il giorno martedì 27 agosto 2019, Aryeh Gregor <[hidden email]> ha scritto:

> I see that in some classes, like WANObjectCache, most methods are declared
> final. Why is this? Is it an attempt to optimize?
>
> The problem is that PHPUnit mocks can't touch final methods. Any ->method()
> calls that try to do anything to them silently do nothing. This makes
> writing tests harder.
>
> If we really want these methods to be marked final for some reason, the
> workaround for PHP is to make an interface that has all the desired
> methods, have the class implement the interface, and make type hints all
> refer to the interface instead of the class. But if there's no good reason
> to declare the methods final to begin with, it's simplest to just drop it.
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l



--
https://meta.wikimedia.org/wiki/User:Daimona_Eaytoy
"Daimona" is not my real name -- he/him
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Declaring methods final in classes

Máté Szabó
Hey,

My understanding is that mocking final methods and types is a limitation that is not specific to just PHPUnit, or indeed to PHP itself. Mockery, another established PHP mock object framework, relies on a workaround for mocking final methods and types that prevents testing code with type constraints or instanceof checks[1]. On the JVM, the popular mocking framework Mockito similarly has to rely on instrumentation to provide support for this use case[2].

Personally, I do not have enough context to judge whether there is added value in declaring those methods in e.g. WANObjectCache as final. It may have been intended to explicitly prevent subclassing and overriding by extension code, although this is just a guess. A question that could be posed is whether these benefits are worth the tradeoff in terms of reduced testability.

What do you think?

——
[1] http://docs.mockery.io/en/latest/reference/final_methods_classes.html
[2] https://static.javadoc.io/org.mockito/mockito-core/3.0.0/org/mockito/Mockito.html#39

Máté Szabó
SOFTWARE ENGINEER
+36 30 947 5903

WIKIA sp. z o.o. z siedzibą w Poznaniu, ul. Abp. A. Baraniaka 6
Sąd Rejonowy Poznań – Nowe Miasto i Wilda w Poznaniu, VIII Wydział Gospodarczy Krajowego Rejestru Sądowego, KRS 0000254365
NIP: 5252358778
Kapitał zakładowy: 50.000,00 złotych


> On 27 Aug 2019, at 20:56, Daimona <[hidden email]> wrote:
>
> Personally, I don't like these limitations in PHPUnit and the like. IMHO,
> they should never be a reason for changing good code. And sometimes,
> methods have to be final. I don't know about the specific case, though.
>
> Anyway, some time ago I came across [1], which allows mocking final methods
> and classes. IIRC, it does that by removing the `final` keywords from the
> tokenized PHP code. I don't know how well it works, nor if it could degrade
> performance, but if it doesn't we could bring it in via composer.
>
>
> [1] - https://github.com/dg/bypass-finals
>
>
> Il giorno martedì 27 agosto 2019, Aryeh Gregor <[hidden email]> ha scritto:
>
>> I see that in some classes, like WANObjectCache, most methods are declared
>> final. Why is this? Is it an attempt to optimize?
>>
>> The problem is that PHPUnit mocks can't touch final methods. Any ->method()
>> calls that try to do anything to them silently do nothing. This makes
>> writing tests harder.
>>
>> If we really want these methods to be marked final for some reason, the
>> workaround for PHP is to make an interface that has all the desired
>> methods, have the class implement the interface, and make type hints all
>> refer to the interface instead of the class. But if there's no good reason
>> to declare the methods final to begin with, it's simplest to just drop it.
>> _______________________________________________
>> Wikitech-l mailing list
>> [hidden email]
>> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
>
>
> --
> https://meta.wikimedia.org/wiki/User:Daimona_Eaytoy
> "Daimona" is not my real name -- he/him
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l


_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Declaring methods final in classes

Aryeh Gregor-4
In reply to this post by Daimona
On Tue, Aug 27, 2019 at 11:53 PM Daimona <[hidden email]> wrote:
> Personally, I don't like these limitations in PHPUnit and the like. IMHO,
> they should never be a reason for changing good code.

I don't like these limitations either, but testing is an integral part
of development, and we need to code in a way that facilitates testing.
In each case we need to make a cost-benefit analysis about what's best
for the project. The question is whether there's any benefit to using
final that outweighs the cost to testability.

> And sometimes, methods have to be final.

Why do methods ever "have" to be final? Someone who installs an
extension accepts that they get whatever behavior changes the
extension makes. If the extension does something we don't want it to,
it will either work or not, but that's the extension's problem.

This is exactly the question: why do we ever want methods to be final?
Is there actually any benefit that outweighs the problems for testing?

> Anyway, some time ago I came across [1], which allows mocking final methods
> and classes. IIRC, it does that by removing the `final` keywords from the
> tokenized PHP code. I don't know how well it works, nor if it could degrade
> performance, but if it doesn't we could bring it in via composer.

That would be a nice solution if it works well. If someone wants to
volunteer to try to get it working, then we won't need to have this
discussion. But until someone does, the question remains.

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Declaring methods final in classes

Daimona
> I don't like these limitations either, but testing is an integral part
> of development, and we need to code in a way that facilitates testing.

This is especially true for e.g. static methods, but here we'd be
renouncing to a possibly useful feature.

> Why do methods ever "have" to be final?

If you want to make sure that any subclass won't ever change the
implementation of a method, and thus all callers know what to expect from
calling a final method.
I see finals as a sort of safeguard to help write better code, like e.g.
typehints.

> That would be a nice solution if it works well. If someone wants to
> volunteer to try to get it working, then we won't need to have this
> discussion. But until someone does, the question remains.

IMHO this would be a perfect compromise. I've filed T231419 for that, and I
also think that before discussing any further, we should try to see if we
can install that tool.

Il giorno mer 28 ago 2019 alle ore 09:30 Aryeh Gregor <[hidden email]> ha
scritto:

> On Tue, Aug 27, 2019 at 11:53 PM Daimona <[hidden email]> wrote:
> > Personally, I don't like these limitations in PHPUnit and the like. IMHO,
> > they should never be a reason for changing good code.
>
> I don't like these limitations either, but testing is an integral part
> of development, and we need to code in a way that facilitates testing.
> In each case we need to make a cost-benefit analysis about what's best
> for the project. The question is whether there's any benefit to using
> final that outweighs the cost to testability.
>
> > And sometimes, methods have to be final.
>
> Why do methods ever "have" to be final? Someone who installs an
> extension accepts that they get whatever behavior changes the
> extension makes. If the extension does something we don't want it to,
> it will either work or not, but that's the extension's problem.
>
> This is exactly the question: why do we ever want methods to be final?
> Is there actually any benefit that outweighs the problems for testing?
>
> > Anyway, some time ago I came across [1], which allows mocking final
> methods
> > and classes. IIRC, it does that by removing the `final` keywords from the
> > tokenized PHP code. I don't know how well it works, nor if it could
> degrade
> > performance, but if it doesn't we could bring it in via composer.
>
> That would be a nice solution if it works well. If someone wants to
> volunteer to try to get it working, then we won't need to have this
> discussion. But until someone does, the question remains.
>
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l



--
https://meta.wikimedia.org/wiki/User:Daimona_Eaytoy
"Daimona" is not my real name -- he/him
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Declaring methods final in classes

Daniel Kinzler-3
I see no good use for final methods or classes. Or rather: I see a very limited
benefit and a pretty massive cost.

Subclassing should be very limited anyway, and even more limited across module
boundaries, which could even be enforced via static analysis. Method contracts
should be enforced by compliance tests. When following these principles, making
methods and classes final has little benefit. Preventing mocking is however a
pretty massive cost.

I'd just remove the final markers. But maybe I'm overlooking something?...

Am 28.08.19 um 10:38 schrieb Daimona:
>> I don't like these limitations either, but testing is an integral part
>> of development, and we need to code in a way that facilitates testing.
>
> This is especially true for e.g. static methods, but here we'd be
> renouncing to a possibly useful feature.

Static code should only be used for pure functions. That's a very limited use case.

>> Why do methods ever "have" to be final?
>
> If you want to make sure that any subclass won't ever change the
> implementation of a method, and thus all callers know what to expect from
> calling a final method.
> I see finals as a sort of safeguard to help write better code, like e.g.
> typehints.

This should eb done by documenting contracts, and having tests ensure compliance
with these contracts. Final methods make callers rely on a specific
implementation, which may still end up changing anyway.

>
>> That would be a nice solution if it works well. If someone wants to
>> volunteer to try to get it working, then we won't need to have this
>> discussion. But until someone does, the question remains.
>
> IMHO this would be a perfect compromise. I've filed T231419 for that, and I
> also think that before discussing any further, we should try to see if we
> can install that tool.

If I understand correctly, this would break as soon as the mock object hits a
type hint of instanceof check. That won't fly.


--
Daniel Kinzler
Principal Software Engineer, Core Platform
Wikimedia Foundation

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Declaring methods final in classes

Daimona
>Subclassing should be very limited anyway, and even more limited across
module
>boundaries

I agree, but it doesn't offer a strict guarantee.

> which could even be enforced via static analysis.

Why not just use final, then?

> Method contracts should be enforced by compliance tests. When following
these principles, making
> methods and classes final has little benefit.

Ideally, yes. But I don't think our codebase has enough tests for that.

> Preventing mocking is however a pretty massive cost.

Definitely yes. But making methods mockable while also keeping the
capability to use finals is even better IMHO.

> Static code should only be used for pure functions. That's a very limited
use case.

In theory, for sure. But I believe there are lots of occurrences in our
code where static methods are not pure functions.

>> If you want to make sure that any subclass won't ever change the
>> implementation of a method, and thus all callers know what to expect from
>> calling a final method.
>> I see finals as a sort of safeguard to help write better code, like e.g.
>> typehints.
>
>This should eb done by documenting contracts, and having tests ensure
compliance
>with these contracts. Final methods make callers rely on a specific
>implementation, which may still end up changing anyway.

Two sides of a coin, I think. Each of them has its benefits and its
drawbacks, I'd say.

>> IMHO this would be a perfect compromise. I've filed T231419 for that,
and I
>> also think that before discussing any further, we should try to see if we
>> can install that tool.
>
>If I understand correctly, this would break as soon as the mock object
hits a
>type hint of instanceof check. That won't fly.

No, that's only what happens with mockery. The tool I found just strips
'final' keywords from the PHP code - I believe, I still haven't looked at
the implementation.


Il giorno mer 28 ago 2019 alle ore 16:29 Daniel Kinzler <
[hidden email]> ha scritto:

> I see no good use for final methods or classes. Or rather: I see a very
> limited
> benefit and a pretty massive cost.
>
> Subclassing should be very limited anyway, and even more limited across
> module
> boundaries, which could even be enforced via static analysis. Method
> contracts
> should be enforced by compliance tests. When following these principles,
> making
> methods and classes final has little benefit. Preventing mocking is
> however a
> pretty massive cost.
>
> I'd just remove the final markers. But maybe I'm overlooking something?...
>
> Am 28.08.19 um 10:38 schrieb Daimona:
> >> I don't like these limitations either, but testing is an integral part
> >> of development, and we need to code in a way that facilitates testing.
> >
> > This is especially true for e.g. static methods, but here we'd be
> > renouncing to a possibly useful feature.
>
> Static code should only be used for pure functions. That's a very limited
> use case.
>
> >> Why do methods ever "have" to be final?
> >
> > If you want to make sure that any subclass won't ever change the
> > implementation of a method, and thus all callers know what to expect from
> > calling a final method.
> > I see finals as a sort of safeguard to help write better code, like e.g.
> > typehints.
>
> This should eb done by documenting contracts, and having tests ensure
> compliance
> with these contracts. Final methods make callers rely on a specific
> implementation, which may still end up changing anyway.
>
> >
> >> That would be a nice solution if it works well. If someone wants to
> >> volunteer to try to get it working, then we won't need to have this
> >> discussion. But until someone does, the question remains.
> >
> > IMHO this would be a perfect compromise. I've filed T231419 for that,
> and I
> > also think that before discussing any further, we should try to see if we
> > can install that tool.
>
> If I understand correctly, this would break as soon as the mock object
> hits a
> type hint of instanceof check. That won't fly.
>
>
> --
> Daniel Kinzler
> Principal Software Engineer, Core Platform
> Wikimedia Foundation
>
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l



--
https://meta.wikimedia.org/wiki/User:Daimona_Eaytoy
"Daimona" is not my real name -- he/him
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Declaring methods final in classes

Lucas Werkmeister
>
> No, that's only what happens with mockery. The tool I found just strips
> 'final' keywords from the PHP code - I believe, I still haven't looked at
> the implementation.


As far as I can tell, it actually strips final tokens from *any* PHP file
that’s read, including by application code. It seems to override the
standard PHP handler for the file:// protocol, and rely on the fact that
the PHP engine also uses that handler to read source code files. (Also, I
think it only applies to .php files and not Apache-style .php.en files, but
I guess that’s not a problem for us.)

Am Mi., 28. Aug. 2019 um 16:49 Uhr schrieb Daimona <[hidden email]>:

> >Subclassing should be very limited anyway, and even more limited across
> module
> >boundaries
>
> I agree, but it doesn't offer a strict guarantee.
>
> > which could even be enforced via static analysis.
>
> Why not just use final, then?
>
> > Method contracts should be enforced by compliance tests. When following
> these principles, making
> > methods and classes final has little benefit.
>
> Ideally, yes. But I don't think our codebase has enough tests for that.
>
> > Preventing mocking is however a pretty massive cost.
>
> Definitely yes. But making methods mockable while also keeping the
> capability to use finals is even better IMHO.
>
> > Static code should only be used for pure functions. That's a very limited
> use case.
>
> In theory, for sure. But I believe there are lots of occurrences in our
> code where static methods are not pure functions.
>
> >> If you want to make sure that any subclass won't ever change the
> >> implementation of a method, and thus all callers know what to expect
> from
> >> calling a final method.
> >> I see finals as a sort of safeguard to help write better code, like e.g.
> >> typehints.
> >
> >This should eb done by documenting contracts, and having tests ensure
> compliance
> >with these contracts. Final methods make callers rely on a specific
> >implementation, which may still end up changing anyway.
>
> Two sides of a coin, I think. Each of them has its benefits and its
> drawbacks, I'd say.
>
> >> IMHO this would be a perfect compromise. I've filed T231419 for that,
> and I
> >> also think that before discussing any further, we should try to see if
> we
> >> can install that tool.
> >
> >If I understand correctly, this would break as soon as the mock object
> hits a
> >type hint of instanceof check. That won't fly.
>
> No, that's only what happens with mockery. The tool I found just strips
> 'final' keywords from the PHP code - I believe, I still haven't looked at
> the implementation.
>
>
> Il giorno mer 28 ago 2019 alle ore 16:29 Daniel Kinzler <
> [hidden email]> ha scritto:
>
> > I see no good use for final methods or classes. Or rather: I see a very
> > limited
> > benefit and a pretty massive cost.
> >
> > Subclassing should be very limited anyway, and even more limited across
> > module
> > boundaries, which could even be enforced via static analysis. Method
> > contracts
> > should be enforced by compliance tests. When following these principles,
> > making
> > methods and classes final has little benefit. Preventing mocking is
> > however a
> > pretty massive cost.
> >
> > I'd just remove the final markers. But maybe I'm overlooking
> something?...
> >
> > Am 28.08.19 um 10:38 schrieb Daimona:
> > >> I don't like these limitations either, but testing is an integral part
> > >> of development, and we need to code in a way that facilitates testing.
> > >
> > > This is especially true for e.g. static methods, but here we'd be
> > > renouncing to a possibly useful feature.
> >
> > Static code should only be used for pure functions. That's a very limited
> > use case.
> >
> > >> Why do methods ever "have" to be final?
> > >
> > > If you want to make sure that any subclass won't ever change the
> > > implementation of a method, and thus all callers know what to expect
> from
> > > calling a final method.
> > > I see finals as a sort of safeguard to help write better code, like
> e.g.
> > > typehints.
> >
> > This should eb done by documenting contracts, and having tests ensure
> > compliance
> > with these contracts. Final methods make callers rely on a specific
> > implementation, which may still end up changing anyway.
> >
> > >
> > >> That would be a nice solution if it works well. If someone wants to
> > >> volunteer to try to get it working, then we won't need to have this
> > >> discussion. But until someone does, the question remains.
> > >
> > > IMHO this would be a perfect compromise. I've filed T231419 for that,
> > and I
> > > also think that before discussing any further, we should try to see if
> we
> > > can install that tool.
> >
> > If I understand correctly, this would break as soon as the mock object
> > hits a
> > type hint of instanceof check. That won't fly.
> >
> >
> > --
> > Daniel Kinzler
> > Principal Software Engineer, Core Platform
> > Wikimedia Foundation
> >
> > _______________________________________________
> > Wikitech-l mailing list
> > [hidden email]
> > https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
>
>
> --
> https://meta.wikimedia.org/wiki/User:Daimona_Eaytoy
> "Daimona" is not my real name -- he/him
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l



--
Lucas Werkmeister (he/er)
Full Stack Developer

Wikimedia Deutschland e. V. | Tempelhofer Ufer 23-24 | 10963 Berlin
Phone: +49 (0)30 219 158 26-0
https://wikimedia.de

Imagine a world in which every single human being can freely share in the
sum of all knowledge. Help us to achieve our vision!
https://spenden.wikimedia.de

Wikimedia Deutschland - Gesellschaft zur Förderung Freien Wissens e. V.
Eingetragen im Vereinsregister des Amtsgerichts Berlin-Charlottenburg unter
der Nummer 23855 B. Als gemeinnützig anerkannt durch das Finanzamt für
Körperschaften I Berlin, Steuernummer 27/029/42207.
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Declaring methods final in classes

Aryeh Gregor-4
On Wed, Aug 28, 2019 at 7:24 PM Lucas Werkmeister
<[hidden email]> wrote:
> As far as I can tell, it actually strips final tokens from *any* PHP file
> that’s read, including by application code.

Yes, but only if you turn it on, and we'd only turn it on for tests.

> It seems to override the
> standard PHP handler for the file:// protocol, and rely on the fact that
> the PHP engine also uses that handler to read source code files.

I wonder how it interacts with an opcode cache. Is the cache going to
return the cached result based on mtime or whatever, meaning you'll
get a random mix of code with and without final and tests might fail
because they got a cached version of the file that wasn't
de-finalized? Or does it somehow know? (I don't see how it would.)

I filed an issue on this:
https://github.com/dg/bypass-finals/issues/12 Assuming it somehow
works with an opcode cache, it shouldn't have to be a huge performance
impact, because the files shouldn't be parsed often.

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Declaring methods final in classes

Daniel Kinzler-3
In reply to this post by Daimona
Am 28.08.19 um 16:48 schrieb Daimona:
>> Subclassing should be very limited anyway, and even more limited across
> module
>> boundaries
>
> I agree, but it doesn't offer a strict guarantee.

Do we need a strict guarantee more than we need unit tests?

>> which could even be enforced via static analysis.
>
> Why not just use final, then?

Because it makes it impossible to write unit tests.

Maybe not impossible with the tool you pointed to. If that thing works, it
becomes: it requires effort to set up the CI infrastructure to allow this to
work, and we don't know who is going to do that, or when.

>> Method contracts should be enforced by compliance tests. When following
> these principles, making
>> methods and classes final has little benefit.
>
> Ideally, yes. But I don't think our codebase has enough tests for that.

That's what we are trying to fix, and final stuff is making it hard.

>> Preventing mocking is however a pretty massive cost.
>
> Definitely yes. But making methods mockable while also keeping the
> capability to use finals is even better IMHO.

If that can be made to work, sure. I'm just saying that an inability to mock far
outweights the potential benefits of declaring things as final.

> In theory, for sure. But I believe there are lots of occurrences in our
> code where static methods are not pure functions.

Which indeed is one of the things we are currently trying to fix, because static
code can't be mocked.

>> with these contracts. Final methods make callers rely on a specific
>> implementation, which may still end up changing anyway.
>
> Two sides of a coin, I think. Each of them has its benefits and its
> drawbacks, I'd say.

What benefits does it have to bind to a specific implementation that is not
guaranteed to stay as it is?

>> If I understand correctly, this would break as soon as the mock object
> hits a
>> type hint of instanceof check. That won't fly.
>
> No, that's only what happens with mockery. The tool I found just strips
> 'final' keywords from the PHP code - I believe, I still haven't looked at
> the implementation.

If somebody is volunteering to do the necessary work in the CI infrastructure, fine.

To me it just seems like just removing the final modifier is the easier and
cheaper solution, and doesn't have any big downside.

--
Daniel Kinzler
Principal Software Engineer, Core Platform
Wikimedia Foundation

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Declaring methods final in classes

Daimona
>> I agree, but it doesn't offer a strict guarantee.
>
> Do we need a strict guarantee more than we need unit tests?

I think we need both. Or rather, we need unit tests more, but if that
doesn't exclude using finals, we can have both.

>> Why not just use final, then?
>
> Because it makes it impossible to write unit tests.
>
> Maybe not impossible with the tool you pointed to. If that thing works, it
> becomes: it requires effort to set up the CI infrastructure to allow this
to
> work, and we don't know who is going to do that, or when.

Ah yes, I'm actually writing with the idea in mind that we'll be able to
set up such a tool. If finals and mocking were mutually exclusive, I'd also
prefer to have better tests.

>> But making methods mockable while also keeping the
>> capability to use finals is even better IMHO.
>
> If that can be made to work, sure. I'm just saying that an inability to
mock far
> outweights the potential benefits of declaring things as final.

Yes, agreed with that.

>> In theory, for sure. But I believe there are lots of occurrences in our
>> code where static methods are not pure functions.
>
> Which indeed is one of the things we are currently trying to fix, because
static
> code can't be mocked.

Indeed, and that's not so bad because it (hopefully) forces people not to
write non-pure static methods.

> Two sides of a coin, I think. Each of them has its benefits and its
> drawbacks, I'd say.
>
> What benefits does it have to bind to a specific implementation that is
not
> guaranteed to stay as it is?

If used properly, the final keyword should be for immutable
implementations. Otherwise, it could be a questionable use case.

> If somebody is volunteering to do the necessary work in the CI
infrastructure, fine.
>
> To me it just seems like just removing the final modifier is the easier
and
> cheaper solution, and doesn't have any big downside.

It depends on how hard is it to set up the library. Ideally, we only have
to find a place where to enable it, nothing more. And I also think it won't
harm existing tests in any way. It doesn't even require a CI change. The
only possible drawback is performance / opcache integration pointed out
by @simetrical. Let me try to upload a test change and see how it goes,
tonight or tomorrow.


Il giorno mer 28 ago 2019 alle ore 18:54 Daniel Kinzler <
[hidden email]> ha scritto:

> Am 28.08.19 um 16:48 schrieb Daimona:
> >> Subclassing should be very limited anyway, and even more limited across
> > module
> >> boundaries
> >
> > I agree, but it doesn't offer a strict guarantee.
>
> Do we need a strict guarantee more than we need unit tests?
>
> >> which could even be enforced via static analysis.
> >
> > Why not just use final, then?
>
> Because it makes it impossible to write unit tests.
>
> Maybe not impossible with the tool you pointed to. If that thing works, it
> becomes: it requires effort to set up the CI infrastructure to allow this
> to
> work, and we don't know who is going to do that, or when.
>
> >> Method contracts should be enforced by compliance tests. When following
> > these principles, making
> >> methods and classes final has little benefit.
> >
> > Ideally, yes. But I don't think our codebase has enough tests for that.
>
> That's what we are trying to fix, and final stuff is making it hard.
>
> >> Preventing mocking is however a pretty massive cost.
> >
> > Definitely yes. But making methods mockable while also keeping the
> > capability to use finals is even better IMHO.
>
> If that can be made to work, sure. I'm just saying that an inability to
> mock far
> outweights the potential benefits of declaring things as final.
>
> > In theory, for sure. But I believe there are lots of occurrences in our
> > code where static methods are not pure functions.
>
> Which indeed is one of the things we are currently trying to fix, because
> static
> code can't be mocked.
>
> >> with these contracts. Final methods make callers rely on a specific
> >> implementation, which may still end up changing anyway.
> >
> > Two sides of a coin, I think. Each of them has its benefits and its
> > drawbacks, I'd say.
>
> What benefits does it have to bind to a specific implementation that is not
> guaranteed to stay as it is?
>
> >> If I understand correctly, this would break as soon as the mock object
> > hits a
> >> type hint of instanceof check. That won't fly.
> >
> > No, that's only what happens with mockery. The tool I found just strips
> > 'final' keywords from the PHP code - I believe, I still haven't looked at
> > the implementation.
>
> If somebody is volunteering to do the necessary work in the CI
> infrastructure, fine.
>
> To me it just seems like just removing the final modifier is the easier and
> cheaper solution, and doesn't have any big downside.
>
> --
> Daniel Kinzler
> Principal Software Engineer, Core Platform
> Wikimedia Foundation
>
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l



--
https://meta.wikimedia.org/wiki/User:Daimona_Eaytoy
"Daimona" is not my real name -- he/him
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Declaring methods final in classes

Daniel Kinzler-3
>> What benefits does it have to bind to a specific implementation that is
> not
>> guaranteed to stay as it is?
>
> If used properly, the final keyword should be for immutable
> implementations. Otherwise, it could be a questionable use case.

I think all the current uses of "final" in MW core are questionable, then...

>> If somebody is volunteering to do the necessary work in the CI
> infrastructure, fine.
>>
>> To me it just seems like just removing the final modifier is the easier
> and
>> cheaper solution, and doesn't have any big downside.
>
> It depends on how hard is it to set up the library. Ideally, we only have
> to find a place where to enable it, nothing more. And I also think it won't
> harm existing tests in any way. It doesn't even require a CI change. The
> only possible drawback is performance / opcache integration pointed out
> by @simetrical. Let me try to upload a test change and see how it goes,
> tonight or tomorrow.

Cool, thank you for looking into this!

Sorry if I sounded very negative. I'm trying to be pragmatic and avoid extra
effort where it is not needed. I appreciate your help with evaluating this!

--
Daniel Kinzler
Principal Software Engineer, Core Platform
Wikimedia Foundation

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Declaring methods final in classes

Krinkle
In reply to this post by Aryeh Gregor-4
On Tue, Aug 27, 2019 at 6:55 PM Aryeh Gregor <[hidden email]> wrote:

> I see that in some classes, like WANObjectCache, most methods are declared
> final. Why is this? [..]
>
> The problem is that PHPUnit mocks can't touch final methods. [..]


What did you want to assert in this test?

I find there is sometimes a tendency for a test to needlessly duplicate the
source code by being too strict on expecting exactly which method is called
to the point that it becomes nothing but a more verbose version of the
source code; always requiring a change to both.

Personally, I prefer a style of testing where it providers a simpler view
of the code. More like a manual of how it should be used, and what
observable outcome it should produce.

I think PHPUnit's assertion helpers for things like method()->once() are
quite useful. But, personally, I almost never need them. And in the few
occasions where I did use them, it's never a private, static or final
method (which can't be mocked). In some cases, I accidentally tried to mock
such method and found every time it was either because of pre-existing
technical debt, or because I misunderstood the code, or because I was
testing arbitrary implementation details.

As such, to perhaps help with the conversation, I'd like to have a
practical example we can look at and compare potential solutions. Perhaps
from WANObjectCache, or perhaps with something else.

-- Timo
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Declaring methods final in classes

Aaron Schulz
In reply to this post by Aryeh Gregor-4
Well, changing something in core and breaking a production extenison doing
something silly can't be waived away with "it's the extension's problem" ;)

I mostly use "final" to enforce a delegation pattern, where only certain
key bits of functionality should be filled in by subclasses. It mostly
comes out of years and years of bad experience with core and extension code
subclassing things in annoying ways that inevitably have to be cleaned up
as a side-task to getting some other feature/refactoring patch to pass CI.
It's a clear way to both document and enforce subclass implementation
points. The only reason not to use it is for tests, and I have removed
"final" before (placed in BagOStuff) when I couldn't come up with another
workaround. Interfaces will not work well for protected methods that need
to be overriden and called by an abstract base class.

If no PHP/PHPUnit fix is coming soon, as a practical matter, I'm sure some
other alternative documentation and naming style pattern could be
standardized so that people actually follow it and don't make annoying and
fragile dependencies.

On Wed, Aug 28, 2019 at 12:30 AM Aryeh Gregor <[hidden email]> wrote:

> On Tue, Aug 27, 2019 at 11:53 PM Daimona <[hidden email]> wrote:
> > Personally, I don't like these limitations in PHPUnit and the like. IMHO,
> > they should never be a reason for changing good code.
>
> I don't like these limitations either, but testing is an integral part
> of development, and we need to code in a way that facilitates testing.
> In each case we need to make a cost-benefit analysis about what's best
> for the project. The question is whether there's any benefit to using
> final that outweighs the cost to testability.
>
> > And sometimes, methods have to be final.
>
> Why do methods ever "have" to be final? Someone who installs an
> extension accepts that they get whatever behavior changes the
> extension makes. If the extension does something we don't want it to,
> it will either work or not, but that's the extension's problem.
>
> This is exactly the question: why do we ever want methods to be final?
> Is there actually any benefit that outweighs the problems for testing?
>
> > Anyway, some time ago I came across [1], which allows mocking final
> methods
> > and classes. IIRC, it does that by removing the `final` keywords from the
> > tokenized PHP code. I don't know how well it works, nor if it could
> degrade
> > performance, but if it doesn't we could bring it in via composer.
>
> That would be a nice solution if it works well. If someone wants to
> volunteer to try to get it working, then we won't need to have this
> discussion. But until someone does, the question remains.
>
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l



--
-Aaron
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Declaring methods final in classes

Daniel Kinzler-3
In reply to this post by Krinkle
> As such, to perhaps help with the conversation, I'd like to have a
> practical example we can look at and compare potential solutions. Perhaps
> from WANObjectCache, or perhaps with something else.

Simetrical can speak on the concrete use case, but I'd like to give my thoughts
on mocking WANObjectCache in general: when writing a unit test, the class under
test should ideally be tested in isolation - that is, any of its dependencies
should be mocked. If the dependency is inconsequential or there is a trivial
implementation available, we don't have to be too strict about this. But complex
dependencies should be avoided in unit tests, otherwise it's not a unit test but
an integration test.

WANObjectCache is 2600 lines of complex code. When testing something that has a
WANObjectCache injected, we should inject a fake WANObjectCache, to preserve the
level of isolation that makes the test a unit test. As long as WANObjectCache's
most important methods, like get(), are final, this is impossible.

I agree by the way that overly specific mocks go against the idea of a unit
test. The test should test the contract, not the implementation. The mock should
provide the minimum functionality needed by the code, it shouldn't assert things
like "get() is called only once" - unless that is indeed part of the contract.

Narrow interfaces help with that. If we had for instance a cache interface that
defined just the get() and set() methods, and that's all the code needs, then we
can just provide a mock for that interface, and we wouldn't have to worry about
WANObjectCache or its final methods at all.

--
Daniel Kinzler
Principal Software Engineer, Core Platform
Wikimedia Foundation

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Declaring methods final in classes

David Causse
On Thu, Aug 29, 2019 at 9:36 AM Daniel Kinzler <[hidden email]>
wrote:

>
> Narrow interfaces help with that. If we had for instance a cache interface
> that
> defined just the get() and set() methods, and that's all the code needs,
> then we
> can just provide a mock for that interface, and we wouldn't have to worry
> about
> WANObjectCache or its final methods at all.
>
>
I think this is the right solution, forbidding one feature of the language
(final) because of the current design of WANObjectCache seems going too far
in my opinion.

--
David Causse
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Declaring methods final in classes

Aryeh Gregor-4
In reply to this post by Krinkle
On Thu, Aug 29, 2019 at 1:02 AM Krinkle <[hidden email]> wrote:
> What did you want to assert in this test?

In a proper unit test, I want to completely replace all non-value
classes with mocks, so that they don't call the actual class' code.
This way I can test the class under test without making assumptions
about other classes' behavior.

This is not possible at all if any method is declared final. As soon
as the class under test calls a final method, you cannot mock the
object. This is without any use of expects() or with() -- even just
method()->willReturn().

> I find there is sometimes a tendency for a test to needlessly duplicate the
> source code by being too strict on expecting exactly which method is called
> to the point that it becomes nothing but a more verbose version of the
> source code; always requiring a change to both.
>
> Personally, I prefer a style of testing where it providers a simpler view
> of the code. More like a manual of how it should be used, and what
> observable outcome it should produce.

The idea of good unit tests is to allow refactoring without having to
worry too much that you're accidentally changing observable behavior
in an undesired way. Ideally, then, any observable behavior should be
tested. Changes in source code that don't affect observable behavior
will never necessitate a change to a test, as long as the test doesn't
cheat with TestingAccessWrapper or such.

This includes tests for corner cases where the original behavior was
never considered or intended. This is obviously less important to test
than basic functionality, but in practice, callers often accidentally
depend on all sorts of incidental implementation details. Thus
ideally, they should be tested. If the test needs to be updated, that
means that some caller somewhere might break, and that should be taken
into consideration.

On Thu, Aug 29, 2019 at 1:12 AM Aaron Schulz <[hidden email]> wrote:
> Interfaces will not work well for protected methods that need
> to be overriden and called by an abstract base class.

If you have an interface that the class implements, then it's possible
to mock the interface instead of the class, and the final method
problem goes away. Of course, your "final" is then not very useful if
someone implements the interface instead of extending the class, but
that shouldn't happen if your base class has a lot of code that
subclasses need.

On Thu, Aug 29, 2019 at 10:37 AM Daniel Kinzler <[hidden email]> wrote:
> Narrow interfaces help with that. If we had for instance a cache interface that
> defined just the get() and set() methods, and that's all the code needs, then we
> can just provide a mock for that interface, and we wouldn't have to worry about
> WANObjectCache or its final methods at all.

Any interface would solve the problem, even if it was just a copy of
all the public methods of WANObjectCache. That would be inelegant, but
another possible solution if we want to keep using final.

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Declaring methods final in classes

Daimona
Note that  https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/533067/ is
now ready for review, and works as expected. That could make this
discussion unnecessary.

Il giorno gio 29 ago 2019 alle ore 14:46 Aryeh Gregor <[hidden email]> ha
scritto:

> On Thu, Aug 29, 2019 at 1:02 AM Krinkle <[hidden email]> wrote:
> > What did you want to assert in this test?
>
> In a proper unit test, I want to completely replace all non-value
> classes with mocks, so that they don't call the actual class' code.
> This way I can test the class under test without making assumptions
> about other classes' behavior.
>
> This is not possible at all if any method is declared final. As soon
> as the class under test calls a final method, you cannot mock the
> object. This is without any use of expects() or with() -- even just
> method()->willReturn().
>
> > I find there is sometimes a tendency for a test to needlessly duplicate
> the
> > source code by being too strict on expecting exactly which method is
> called
> > to the point that it becomes nothing but a more verbose version of the
> > source code; always requiring a change to both.
> >
> > Personally, I prefer a style of testing where it providers a simpler view
> > of the code. More like a manual of how it should be used, and what
> > observable outcome it should produce.
>
> The idea of good unit tests is to allow refactoring without having to
> worry too much that you're accidentally changing observable behavior
> in an undesired way. Ideally, then, any observable behavior should be
> tested. Changes in source code that don't affect observable behavior
> will never necessitate a change to a test, as long as the test doesn't
> cheat with TestingAccessWrapper or such.
>
> This includes tests for corner cases where the original behavior was
> never considered or intended. This is obviously less important to test
> than basic functionality, but in practice, callers often accidentally
> depend on all sorts of incidental implementation details. Thus
> ideally, they should be tested. If the test needs to be updated, that
> means that some caller somewhere might break, and that should be taken
> into consideration.
>
> On Thu, Aug 29, 2019 at 1:12 AM Aaron Schulz <[hidden email]>
> wrote:
> > Interfaces will not work well for protected methods that need
> > to be overriden and called by an abstract base class.
>
> If you have an interface that the class implements, then it's possible
> to mock the interface instead of the class, and the final method
> problem goes away. Of course, your "final" is then not very useful if
> someone implements the interface instead of extending the class, but
> that shouldn't happen if your base class has a lot of code that
> subclasses need.
>
> On Thu, Aug 29, 2019 at 10:37 AM Daniel Kinzler <[hidden email]>
> wrote:
> > Narrow interfaces help with that. If we had for instance a cache
> interface that
> > defined just the get() and set() methods, and that's all the code needs,
> then we
> > can just provide a mock for that interface, and we wouldn't have to
> worry about
> > WANObjectCache or its final methods at all.
>
> Any interface would solve the problem, even if it was just a copy of
> all the public methods of WANObjectCache. That would be inelegant, but
> another possible solution if we want to keep using final.
>
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l



--
https://meta.wikimedia.org/wiki/User:Daimona_Eaytoy
"Daimona" is not my real name -- he/him
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Declaring methods final in classes

Adam Wight-4
In reply to this post by Daniel Kinzler-3
On Wed, Aug 28, 2019 at 4:29 PM Daniel Kinzler <[hidden email]>
wrote:

> Subclassing should be very limited anyway, and even more limited across
> module
> boundaries [...]


This is a solution I'd love to see explored.  Subclassing is now considered
harmful, and it would be possible to refactor existing cases to use
interfaces, traits, composition, etc.

-Adam
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Declaring methods final in classes

Daniel Kinzler-3
Am 29.08.19 um 15:31 schrieb Adam Wight:
> This is a solution I'd love to see explored.  Subclassing is now considered
> harmful, and it would be possible to refactor existing cases to use
> interfaces, traits, composition, etc.

I wouldn't say subclassing is considered harmful in all cases. In fact, for
extension points, subclassing is preferred over directly implementing
interfaces. But subclassing should be used very carefully. It's the most tight
form of coupling. It should be avoided if an alternative is readily available.

But subclassing across module boundaries should be restricted to classes
explicitly documented to act as extension points. If we could enforce this
automatically, that would be excellent.

--
Daniel Kinzler
Principal Software Engineer, Core Platform
Wikimedia Foundation

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
12