Extensibility and Flexibility through inheritance

[spawnia]

This is a continuation of the discussion that originated in #592 (comment). To recap:

This article by Fabian Potencier sums up the point in discussion quite nicely: http://fabien.potencier.org/pragmatism-over-theory-protected-vs-private.html

We can lock everything down to the max, making all classes final and methods preferrably private. This does prevent misuse and keeps the API surface small, but hurts flexibility and extensibility. I personally think this approach makes more sense in large project codebases where you want to keep core components clean and limit technical debt. Libraries don't really have that issue.

The other way would be to keep classes open, allowing extending them and overwriting or calling protected methods. The BC guarantees are only upheld for the parts of the library marked with @api. While this provides some footguns and may cause pain for developers who choose to mess with the insides of this library, i think that this is not really a drawback from a library perspective. It gives more power and responsibility to users of this library.

There is also a middle ground, in which we decide on a case-by-case basis and differentiate between protected and private, static and self and deliberately use final.

I think we should choose consciously, as it is an important design decision.

[mfn]

From the linked article:

To summarize my point of view:

"private" is for purists and "protected" is for pragmatic developers.

I like that and I like it if libraries (like this one) are pragmatic 👍

---

Same e.g. with Laravel, almost no private methods and that's really great to achieve certain extensions which otherwise would be more laborious work (i.e. completely duplicate code).

Of course when upgrading such projects you're always at the peril of BC changes but in my experience having tests for this particular code can catch this.

I'm using private-first in all company/proprietary (non-library) code because it's a way of communicating with other/future developers what the intention here is. But that's a different topic because usually if needed, they can just be changed at will anyway.

[spawnia]

I think that private methods/properties should be used sparingly. They can make sense when you are very certain that you want to totally encapsulate some behaviour and have provided a complete abstraction over its behaviour. Another use case would be to hide some ugly code that is deemed a workaround and you already know you wan't to change it soon.

The only real use-case i found for final classes is enum-like classes where other classes specifically depend on a fixed number of defined const.

[mfn]

Well put, can't agree more 👍

[vladar]

Good point. But I feel that we need some specific examples from this lib to make this discussion more specific.

[spawnia]

I gathered some statistics on how open/closed the code currently is.

	self	static
Instantiation with new	12	8
Access ::SOME_CONST	178	0
Call ::someFunc()	1824	62

	private	protected
function	344	48
static function	65	22
property	202	13
static property	46	4

There are 2 final classes (and 5 final test classes).

Just on a glance, it seems like we might be able to improve upon the consistency. The usage of access modifier looks somewhat random, although i am sure that thought and care has be put into it.

Generally, i see a tendency towards closing the classes for modification, which might be driven by static analysers suggesting such approaches.

I am not saying that committing 100% to one certain approach is correct, but rather that it will be useful to normalize an organically grown codebase across some axis.

[spawnia]

In terms of specific examples, i can recall two cases where the closed-ness of classes has made it difficult for me to extend them and i ended up re-implementing most of their logic and duplicating a lot of code.

• Partial parsing of SDL fragments: While we have implemented such functionality in the Parser now, before i had to resort to some ugly workarounds, see Parse partial source by delegating calls to the internal parseX methods of the Parser #501. Had the parser's internal methods been protected, i could have added that functionality myself. On the flipside, the imposed limitation lead to the implementation of a well-defined extension point.

• Including directives in the schema printer (Include directives in SchemaPrinter output #552): As long as such functionality is not added as an option to the schema printer itself, there is no practical way to extend it and customize such needed functionality.

[vladar]

Yeah but sometimes protected is not enough to actually extend something. You may also want factories to replace existing instances with your implementations.

In general, nominal type systems and inheritance are like a rabbit hole where you start with something simple (i.e. protected or introducing more interfaces) but have to add more and more layers of complexity just to make them work as intended.

I do agree that the pragmatic approach makes sense but I don't want to jump into the IoC madness too. So yeah, it should be somewhat pragmatical - we should identify places where extending things should be enough (like lots of Utils classes and maybe Parser/Printer).

But I suspect it won't work for everything out there.

[renepardon]

Is there an update on this? Did I missed something? Are more methods extendable now? :)

Implementing Apollo federation support into laravel lighthouse package is not possible right now without duplicating the Schema Printer and it's dependencies. This is not really a solution and error prone.

[spawnia]

@renepardon If there is a specific use case, such as extending the schema printer, you can create a PR for it and make the required methods protected.

[spawnia]

I think the discussion has been had, this issue by itself is no longer actionable.