setDefaultValueCallback() & setDefaultValue() should be on FieldDefinitionInterface, not FieldConfigInterface

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

Seems this survived the review queue purge.

So to continue the conversation #8 should this be a documentation update?

If not Patch for #3
- New functions and properties should be typehinted for D10
- Needs a test case to show the issue

Did not test for the issue locally

Thanks!

Came up as a random BSI triage. As per #8 we can rescope this as a documentation issue.

This looks correct to me

Looked at other uses of mixed $value when reading that section and seemed to line up there too.

LGTM

The docs themselves look great and appear to be accurate. I added one suggestion.

One thing though: Are there other places this is documented in core, and are we repeating documentation? Could we use an @see?

And if we haven't checked for that, it would be good to look up the documentation of e.g. callers.

Thanks!

Added code review: callables: called or invoked.

Saving reviewer credits.

@xjm If you are unconvinced by my arguments (code review), do you want to do a follow-up to change the few instances of 'calling' callables' in the existing Drupal codebase to 'invoking'?

@xjm You are correct that in Drupal API docs eg:
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension...

there are:

callable $callback: A callable that invokes a hook implementation.

But is that the purpose of the callback in this issue?

If not then best to contradistinguish the chalk from the cheese?

@oily, Thanks for your interest in this topic. The discussion is out of scope for this issue, so please create a new issue to discuss it. Thanks!

Reference: Drupal core issue scope guidelines →

The outstanding tasks are as I posted in my previous committer review:

One thing though: Are there other places this is documented in core, and are we repeating documentation? Could we use an @see?

And if we haven't checked for that, it would be good to look up the documentation of e.g. callers.

While this still set to NW, I'm going to nitpick and say that the NULL case should be a first-level thing, not a parenthetical.

But other than that, it's looking really good!

#31 @xjm

@oily, Thanks for your interest in this topic.

Everyone else has 'contributed' to this discussion.

... the discussion is out of scope for this issue

How is it 'out of scope'?!

Re #32 "Plus addressing @joachim's feedback". So why not addressing my 'feedback', too?

@oily, this is why I've ruled your feedback as out of scope:

[ayrton:drupal | Sun 13:28:30] $ grep -r "invoke" * | grep "callback" | grep -v "vendor" | grep -v "node_modules" | wc -l
      31

This even includes things that aren't documentation at all:

core/lib/Drupal/Core/Theme/ThemeManager.php:    $invoke_preprocess_callback = function (mixed $preprocessor_function) use ($invoke_map, &$variables, $hook, $info): mixed

As a Drupal core release manager, I have final decision-making over issue scope and management decisions of this sort. Like I've stated, you have valid points about the terminology, but since this is a pattern already used widely in both core docs and runtime code, your proposal to improve the usage should be made as a coding standards proposal, not an argument about a single word in a docblock for an otherwise nearly committable issue. Please do not post any further comments about this on this issue.

I've added suggestions to address @joachim's suggestion in #33 and on the MR regarding the static::.

Thank you for #36 @xjm. Please refer to #30:

A PHP callable is a PHP variable that can be used by the call_user_func() function and returns true when passed to the is_callable() function. It can be a \Closure instance, an object implementing an __invoke() method (which is what closures are in fact), a string representing a function or an array representing an object method or a class method.,

That seems to be a clear description of PHP callables. It explains the variance in Drupal docs you have highlighted in #36.

The action to take when documenting a callable is straightforward:

1. Is it a \Closure? If so it must be an object implementing an __invoke() method. So it is 'invoked'..
2. If not a \Closure and does not invoke an __invoke() method, is may be a string representing a function?
3. If neither of the above, is must be an array?

Line 508
throw new \InvalidArgumentException('Default value callback must be a <strong>string</strong>,...

makes it clear that the $callback parameter of BaseFieldDefinition::setDefaultValueCallback() is a string callback (see item 2).

Therefore, it is 'called' not 'invoked'. Callables elsewhere e.g. in Drupal API docs (see item 1) are correctly described as 'invoked'.

If the callback at line 493 is 'invoked' then it should be typehinted as \Closure, not string|NULL.

@oily, you have made your point repeatedly. Please file a separate issue. Thanks.

@xjm Thank you for #42 but I respectfully disagree: I have been constantly changing my 'point'. It has evolved.

The status is 'Needs review'. Distilled from the above is this:

At line 493:

string|NULL should be callable|NULL

If others disagree then I am keen to hear their viewpoint.

Not sure why pipeline failed. Failures dont seem related. It has failed on a one word commit.

@oily check your changes, you've removed a chunk of code.

@acbramley I used the online single file editor. Weird.

Pipleline green.

Change to Needs review.

How come we went ahead with the change when there wasn’t consensus and even mentioned a separate issue in #42?

#49 @smustgrave Because the typehint should not be 'string' it should be 'callable'. If you disagree, please explain why.

Just saying it’s best to get consensus before just making the change

@smustgrave. I agree. I created a code review as others here did. My review was to change 'string' to 'callable'. I believe my review was ignored while everyone else's was resolved. No idea why.