Skip to content

Move field methods to Content\Field class #7082

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 4 commits into
base: v5/develop
Choose a base branch
from
Draft

Move field methods to Content\Field class #7082

wants to merge 4 commits into from

Conversation

distantnative
Copy link
Member

@distantnative distantnative commented Mar 20, 2025 •
edited
Loading

Description

Todo

  • Ensure altering methods return clone, not alter current object
  • No way to overwrite default field methods anymore - an issue?
    • Was possible before, though documented that one cannot do this
  • What Kirby version should/could this be part of?
  • 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()

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

  • 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

@distantnative distantnative self-assigned this Mar 20, 2025
@distantnative distantnative linked an issue Mar 20, 2025 that may be closed by this pull request
Code completion for field methods #7078
Open
@distantnative distantnative force-pushed the v5/refactor/field-methods branch from 6f3d1fb to 55012fa Compare March 20, 2025 21:22
@distantnative distantnative force-pushed the v5/refactor/field-methods branch from 55012fa to d02ec9c Compare March 20, 2025 21:29
@fvsch
Copy link

fvsch commented Mar 21, 2025 •
edited
Loading

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
Copy link
Member

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

@fvsch
Copy link

fvsch commented Mar 21, 2025

@bastianallgeier Ha! I did not know that. 😅

@distantnative distantnative added this to the 6.0.0-alpha.2 milestone Apr 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@distantnative distantnative

Labels
None yet
Milestone
6.0.0-alpha.2
Development

Successfully merging this pull request may close these issues.

Code completion for field methods
3 participants
@distantnative @fvsch @bastianallgeier