Skip to content

Code completion for field methods #7078

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.

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

Open
fvsch opened this issue Mar 19, 2025 · 4 comments · May be fixed by #7082
Open

Code completion for field methods #7078

fvsch opened this issue Mar 19, 2025 · 4 comments · May be fixed by #7082
Assignees

Comments

@fvsch
Copy link

fvsch commented Mar 19, 2025

The problem

The Kirby\Content\Field class is a type returned by common templating methods such as $page->content()->get('field_name').

This object supports 56 methods documented here.

But in an IDE, only a few methods (10) are visible, because only 10 methods are statistically declared by the Field class (generated docs). The other 46 are created here instead:
https://github.com/getkirby/kirby/blob/5.0.0-beta.4/config/methods.php

This means that most helper methods one might want to use with fields show an error in an IDE with any PHP language server. For instance, in Zed with the phpactor plugin:

Screenshot of the Zed editor with a $field variable set to a Kirby\Content\Field object, then a couple method calls like exists() and isNotEmpty() showing no error, and a couple others like toBlocks() and kirbytext() being underlined in red. An error tooltip says: phpactor: Method 'toBlocks' does not exist on class 'Kirby\Content\Field'

This makes it needlessly hard to work with content fields in PHP code, since you have to rely on the documentation exclusively.

Expected outcome

When working with a Kirby\Content\Field instance, all utility methods listed in the docs should be visible in the IDE's code completion.

Out of scope: field methods provided by extensions would not be listed.

Possible solutions

  1. Refactor config/methods.php to move all the built-in methods directly to the Kirby\Content\Field class.
  2. Add 40+ PHPDoc @method tags to the Kirby\Content\Field class.
  3. Declare all the built-in methods as public methods of Kirby\Content\Field, but call the current implementation:
// src/Content/Field.php

class Field implements Stringable
{
	// ...

	/**
	 * Splits the field content into an array
	 */
	public function split(string $separator = ','): array
	{
		return $this->__call('split', [$separator]);
	}

}

The first option would be ideal, but is a bigger refactoring which might have some technical challenges and more technical risk.

The second option has no risk of breakage, but risks getting out of sync with the actual implementation. Because it uses the PHPDoc @method tag, it's a bit limited in what information it can convey (vs what a dedicated PHPDoc comment could contain).

The third option is a variant of the second one, and has much of the same tradeoffs. It's a bit nicer to author, and the IDE's jump-to-implementation is just a tad nicer.

@fvsch
Copy link
Author

fvsch commented Mar 19, 2025

I'd be happy to work on this.

My preferred approach would be to migrate all the built-in field methods to the Kirby\Content\Field class. It looks like the code that registers "system" field methods wouldn't be too hard to change either.

The biggest difficulty seems to be that ~10 of those methods, such as the Markdown and KirbyText-related ones, need access to the Kirby\Cms\App instance. Currently they close over an $app variable passed when registering those methods.

When a Field is instantiated with a $parent argument (Kirby\Cms\ModelWithContent), a method of Field could use $this->parent()->kirby() to get the App instance. But for fields which don't have a parent, I'm not sure what the right approach would be. Would it be okay to use App::instance() in that context?

@distantnative
Copy link
Member

Don't start working on this cause I have a local branch very far along on this already.

@distantnative distantnative self-assigned this Mar 19, 2025
@distantnative
Copy link
Member

distantnative commented Mar 20, 2025

This is the branch where I moved all methods to the Content\Field class: https://github.com/getkirby/kirby/tree/lab/nicofield-methods

A major roadblock for this is the current possibility to overwrite/extend core field methods. I don't have a good idea yet how/if this could work when we move all field methods to the Content\Field class which would be the preferred solution.

If we don't find a solution for that, it would be either adding a big breaking change or shifting to solution 2 (@method tags) which isn't great to maintain.

@distantnative distantnative added the needs: help 🙏 Really needs some help on this label Mar 20, 2025
@distantnative distantnative changed the title Improve code completion for field methods (Kirby\Content\Field) Code completion for field methods Mar 20, 2025
@fvsch
Copy link
Author

fvsch commented Mar 20, 2025

A major roadblock for this is the current possibility to overwrite/extend core field methods.

The reference currently says that extensions cannot override the default Field methods, and links to the longer list of field methods:
https://getkirby.com/docs/reference/plugins/extensions/field-methods#default-field-methods

Thought after looking at the source code of Kirby\Cms\App and Kirby\Cms\AppPlugins, it looks like built-in methods would be registered before extension methods, so extension methods would indeed override built-in methods except the ones which are already statically defined (like Field::parent or Field::isNotEmpty).

I don't have a good idea yet how/if this could work when we move all field methods to the Content\Field class which would be the preferred solution.

One solution would be to define every overridable method with a condition that checks for the presence of a registered method:

class Field implements Stringable
{
	// (…)
	
	public function toEntries(Field $field): Collection
	{
		if ($this->hasMethodOverride('toEntries')) {
			return $this->__call('toEntries');
		}
		// (…) Default implementation here
	}

	public function toFiles(string $separator = 'yaml')
	{
		if ($this->hasMethodOverride('toFiles')) {
			return $this->__call('toFiles', [$separator]);
		}
		// (…) Default implementation here
	}
}

It requires a bit of boilerplate but looks manageable. (The boilerplate could be reduced with something like decorators, but it looks like PHP8's Attributes don't really work as decorators.)

@distantnative distantnative linked a pull request Mar 20, 2025 that will close this issue
6 tasks
@distantnative distantnative removed the needs: help 🙏 Really needs some help on this label May 10, 2025
distantnative added a commit that referenced this issue May 10, 2025
Resolves #7078
BREAKING CHANGE: Removed `Kirby\Content\Field::$aliases`.
BREAKING CHANGE: Removed `Kirby\Cms\Core::fieldMethods()` and `Kirby\Cms\Core::fieldMethodsAliases()`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants