Remove "final" from methods in Table schema class

[aimeos]

Feature Request

What

There are only two methods in the Table schema class which are marked as final, which seems to make no sense:

dbal/src/Schema/Table.php

Lines 274 to 283 in afcd624

	final public function getRenamedColumns(): array
	{
	return $this->renamedColumns;
	}
	/**
	* @throws LogicException
	* @throws SchemaException
	*/
	final public function renameColumn(string $oldName, string $newName): Column

Why

There seems to be no valid reason to make that methods final and using final methods, this makes it impossible to a) overwrite these methods and b) mock them in tests (our problem at the moment).

How

Just remove the "final" method modifier here:

dbal/src/Schema/Table.php

Lines 274 to 283 in afcd624

	final public function getRenamedColumns(): array
	{
	return $this->renamedColumns;
	}
	/**
	* @throws LogicException
	* @throws SchemaException
	*/
	final public function renameColumn(string $oldName, string $newName): Column

[derrabus]

To be honest, I don't find this kind of issue very constructive. The fact that you don't know the reason behind a developer's decision, does not mean that there is "no valid reason" for that decision or that said decision "makes no sense". Just as you probably wouldn't be happy about being told that mocking a state representation of a 3rd party package such as the Table class makes no sense and that there's no valid reason to create such a mock.

[aimeos]

Sorry for my ignorance questioning that decision. What is the reason behind using final for those two methods only?

[morozov]

We are trying to keep the external API surface to the necessary minimum to reduce the amount of effort required for maintenance.

[greg0ire]

Regarding the "only" part, that's because they were added after the other methods, that were historically not final. Removing final is not a breaking change, but adding it is.

[aimeos]

@morozov @greg0ire Thank you for your explanation!

Nevertheless, adding final to a public class method doesn't reduce the API surface and maintenance as the method can be used by everyone which makes it impossible to change its name, parameters or return value without breaking external code (and therefore, introduce a new major version).

According to https://ocramius.github.io/blog/when-to-declare-classes-final/, final should only be used if there's an interface which contains all public methods of the class, so at the moment, using final for those two class methods only makes testing with PHPUnit using stubs impossible.

[greg0ire]

Nevertheless, adding final to a public class method doesn't reduce the API surface and maintenance as the method can be used by everyone which makes it impossible to change its name, parameters or return value without breaking external code (and therefore, introduce a new major version).

That's where you are wrong. For instance, this is a breaking change for extending types:

-protected function();
+protected function(): int;

add the final keyword, and it no longer is a breaking change:

-final protected function();
+final protected function(): int;

Having the possibility to do such a change in the future could be handy in case generics are introduced, for instance.

According to https://ocramius.github.io/blog/when-to-declare-classes-final/, final should only be used if there's an interface which contains all public methods of the class, so at the moment, using final for those two class methods only makes testing with PHPUnit using stubs impossible.

Then don't use stubs? It's not like creating a Table is hard, is it? Not only is it easy, it's IMO a better practice than using a stub for something you do not own (as already pointed out by @derrabus).

[morozov]

Another problem with non-final methods is that, if we deprecate one, we need to have the framework use it until the next major release, because if it's overridden, then no longer calling it is a BC break.

Eventually, all subclasses of AbstractAsset will become proper value objects (final and immutable). To test against a given value, one needs to construct it.

[aimeos]

Another problem with non-final methods is that, if we deprecate one, we need to have the framework use it until the next major release, because if it's overridden, then no longer calling it is a BC break.

You can't remove public methods (final or not) without a BC break.

[greg0ire]

Where did you read anything about removing a method?

[aimeos]

Public class methods belong to the external API and can be used by consumers of the API, so any removal of a public class method is a BC break

[greg0ire]

Yes, but nobody here said anything about removing them.

[aimeos]

Another problem with non-final methods is that, if we deprecate one, we need to have the framework use it until the next major release, because if it's overridden, then no longer calling it is a BC break.

Here

[greg0ire]

No? This is about being able to stop calling the deprecated method… what makes you think it's about removing it?