refact!: Move field methods to Content\Field

[distantnative]

Description

Discuss

• No way to overwrite default field methods anymore - an issue?
  • Was possible before, though documented that one cannot do this
• Cookbook article defines custom replace method even though this is in the core. Change?

Summary of changes

• Moved all field methods from config/methods.php into Kirby\Content\Field class
• Removed Kirby\Content\Field::$aliases array and instead added aliases as actual methods that call the main methods
• Callback passed to Field::value($value) is now bound to the cloned new field object
• Merged unit test classes and sorted them alphabetically
• Removed Core::fieldMethods() and Core::fieldMethodsAliases()
• Calling an unknown field method now throws an exception

Reasoning

Allows IDEs etc. to actually "understand" field methods, their parameter and return type hints etc.

Changelog

Refactoring

• Moved default field methods into Kirby\Content\Field

Breaking changes

• Calling non-existing field methods is now throwing a Kirby\Exception\BadMethodCallException
• Removed Kirby\Content\Field::$aliases
• Removed Kirby\Cms\Core::fieldMethods() and Kirby\Cms\Core::fieldMethodsAliases()

Ready?

• In-code documentation (wherever needed)
• Unit tests for fixed bug/feature
• Tests and CI checks all pass

For review team

• Add changes & docs to release notes draft in Notion

[fvsch]

Not relevant, comment was based on incorrect assumption

One behavior change not listed in the TODO section for this PR:

Currently, method access with __call is case-insensitive:

1. All 'fieldMethods' are registered with their name converted to lowercase (including the built-in ones, so 'isValid' is documented as $field->isValid(…) but registered with the name 'isvalid'; and an extension could override it by defining 'isValid', 'isvalid', 'ISVALID', etc.).
2. Then they are accessed through __call with the method name also converted to lowercase.

This means that currently it's possible to write $field->kirbytext() or $field->kirbyText() (or $field->KirbyTEXT(), etc.) and get the same result, but with this change users will have to make sure to use $field->kirbytext(), which is a breaking change.

A way to keep this backwards-compatible would be to have a mapping of public methods, then in __call to check if one of those needs to be called (NB: pseudoish code, I don't write a lot of PHP so some syntax may be off):

class Field
{
	private static $builtinMethods = [
		'isvalid' => 'isValid',
		'kirbytext' => 'kirbytext',
		// …
	];

	public function __call(string $method, array $arguments = []): mixed
	{
		$method = strtolower($method);

		if ($this->hasMethod($method) === true) {
			return $this->callMethod($method, [clone $this, ...$arguments]);
		} else if (isset(static::$builtinMethods[$method])) {
			return $this->{static::$builtinMethods[$method]}(...$arguments);
		} else {
			// TODO: throw deprecation, then exception
			// when unknown method is called
		}

		return $this;
	}
}

(Alternatively, some of that logic — or something a bit more elaborate — could move to the HasMethods trait, if it's used in other classes which would need similar capabilities.)

[bastianallgeier]

@fvsch this is not needed. PHP Methods are case insensitive by default.

[fvsch]

@bastianallgeier Ha! I did not know that. 😅

[distantnative]

Explanation: I corrected these, as calling the non-existent field method $field->foo() would just return $field anyways. V::size() itself takes care of applying the ->value() field method.

[distantnative]

Comment: Alternatively to deleting the file we could add some legacy code to ease breaking changes where users require the config file to extend certain field methods. But not sure if this really helps or just delays the pain.

'isFalse' => fn (Field $field) => $field->isFalse()

[bastianallgeier]

When we no longer allow to overwrite field methods, extending should not be an issue, right? It feels pretty dirty to keep all the old code around for a very unlikely scenario. If we see that a lot of people need it during the alpha or beta, we could still do that.

[bastianallgeier]

Finally coming back to this and my first thought was that we could move all the field methods into a FieldMethods trait or maybe into multiple ones by group. I think having them all in a single file is quite overwhelming when we have to deal with core field logic.

[distantnative]

@bastianallgeier Moved them into a FieldMethod trait.

[bastianallgeier]

I keep on thinking about how we could potentially have custom methods also in a trait or custom class, but I guess there really is no good solution for this.

[distantnative]

@bastianallgeier I think there would be a way but not sure that's actually more helpful for developers: HasMethods could support class names for its static $methods prop as well. And in ::getMethods() loop through the registered classes to find the method in one of them. But where would you see the advantage?

[bastianallgeier]

There's none if we don't manage to get IDE support through that.

[lukasbestle]

Questions: How is this related to the field method rewrite? Do we really need to swallow all throwables here?

If it's just about ->id() potentially failing and there is no other fix for that, I think it would be safer to swallow throwables only for that call, not for the whole parent::append() call.

[lukasbestle]

I think everything we would do at runtime would have a really (really) hard time of being picked up by IDEs. And since plugins need to register their extensions, the only way to pick those up would be a custom Kirby extension for IDE language servers. By that point, it would not really matter how we implement it in Kirby. Runtime stays runtime. So I think there's nothing we can really do there.

About overwriting core methods: I would assume that some devs have done that despite the docs. But I guess we will see during the pre-release phase if it will be a notable problem. We could still go with Florens' suggested solution for specific tags should it become relevant.