Handle any callable for options allowed_values_function

Just curious, should we have a is_callable in the if statement?

+++ b/core/modules/options/options.module
@@ -84,8 +84,8 @@ function options_allowed_values(FieldStorageDefinitionInterface $definition, Fie
+      $values = call_user_func_array($function, [$definition, $entity, &$cacheable]);

Oh passing a parameter by reference is still a thing?

This is just me, but I disagree with the is_callable. You can only ever set allowed_values_function from code, so even if it ends up in code, there's really no user interaction that could cause a failure. So if the allowed_values_function is not a callable it's either a straight up developer error (typo, ...) or it's a problem with the setup (borked autoloader, module not installed, ...). In both cases I'd rather have a fatal, than use a potentially bogus default value, so I can actually fix the problem.

Leaving at needs review to get more opinions on this.

I agree, well ideally we would check it and otherwise throw an exception, but I agree, there is no much difference to a call to undefined function.

well ideally we would check it and otherwise throw an exception

I would totally dig that, but yeah, not sure whether the twisted joy that I feel when I see typed exceptions bubble up to my browser is worth coding and maintaining those extra LOCs.

Why not, its just 3 lines.

+++ b/core/modules/options/options.module
@@ -85,7 +84,12 @@ function options_allowed_values(FieldStorageDefinitionInterface $definition, Fie
+        throw new Exception($function . " is not callable.");

Are we sure $function can be cast to a string?

OOOHHH
I didn't knew is_callable() has this functionality!

This patch has the nice side effect of clearing up a PHPStorm error that the entire file is invalid due to the setting method returning mixed and not a callable.

1. +++ b/core/modules/options/options.module
   @@ -62,11 +62,10 @@ function options_field_storage_config_delete(FieldStorageConfigInterface $field_
   - *
   

   Code style, we need this blank line and the one between @return and @throws.

2. +++ b/core/modules/options/options.module
   @@ -85,7 +84,12 @@ function options_allowed_values(FieldStorageDefinitionInterface $definition, Fie
   +        throw new Exception($callable_name . " is not callable.");
   

   Should we throw http://php.net/manual/en/class.badfunctioncallexception.php or http://php.net/manual/en/class.badmethodcallexception.php instead?

3. +++ b/core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php
   @@ -63,17 +63,30 @@ public function getSettableValues(AccountInterface $account = NULL) {
   +      watchdog_exception('options', $e);
   

   This I think highlights why we should throw an exception above. Let's say the callback throws it's own exception, it's going to be silently logged instead of giving any other code a chance to deal with it.

   I would say we should either catch a specific exception (possibly even subclassing one of the above) or remove the catches entirely.

Agreed, this looks good to me.

+++ b/core/modules/options/options.module
@@ -85,7 +87,12 @@ function options_allowed_values(FieldStorageDefinitionInterface $definition, Fie
+        throw new BadFunctionCallException($callable_name . " is not callable.");

Needs a leading slash and exception messages shouldn't end in a fullstop because they appear as part of the sentence when displayed. And it probably could be tested.

Given this is a bug fix and actually just provides a nicer way to break this is definitely eligible for 8.1.x

Hi,
As per the Comment #20 made a small change in timmillwood's patch.

Patch looks great, thanks @arunkumark!

Still needs tests, though, so setting back to "needs work" for now, now that the test bot has confirmed the patch is good.

#20 was true during the beta phase for 8.1.x, but since the fix changes the error flow, we should target it for the development minor after the first release candidate of the previous minor. (I.e., it's not patch-eligible.)

Agreed on needing tests; they should be pretty simple to write I think?

Thanks everyone!

@xjm: Can you elaborate on what you mean by "changes the error flow"? Seems to me this is purely a DX-nicety and, thus, 8.2.x eligible (both before and after 8.2.0).

I think @xjm points to the different of providing a fatal: call to undefined function vs. throwing an exception. As long we are not in PHP7 land, where those errors are maybe handled in a better way, replacing a fatal error with an exception is really the only sane way of doing it. One thing I would try to figure out: What do other places in core do here? Do they silently fail and just don't call the function (the render/theme/form systems should have a bunch of those examples). IMHO we should follow that example.

@dawhener looking at the renderer it would just fatal in \Drupal\Core\Render\Renderer::render()

    // Make any final changes to the element before it is rendered. This means
    // that the $element or the children can be altered or corrected before the
    // element is rendered into the final text.
    if (isset($elements['#pre_render'])) {
      foreach ($elements['#pre_render'] as $callable) {
        if (is_string($callable) && strpos($callable, '::') === FALSE) {
          $callable = $this->controllerResolver->getControllerFromDefinition($callable);
        }
        $elements = call_user_func($callable, $elements);
      }
    }

Throwing the exception still seems better than a fatal since we can test that behavior.

Note - patch #22 still applies to 8.4.x

Throwing the exception still seems better than a fatal since we can test that behavior.

Yeah a fatal is never what you want, IMHO.

Setting back to needs review, as the patch passes.

closed this other as duplicate: #2547505: "allowed_values_function" doesn't use call_user_func()

Note from discussion with Berdir in IRC, you can only set in the field config a string for this, so you can use a string or static method but not an array.

If you try to save a field storage definition with an array callable you get an exception. So, it's probably fine to support any callable, but in practice (and for tests) we need only consider strings.

Here's with some added tests.

Re rolled.

+++ b/core/modules/options/options.module
@@ -67,6 +67,8 @@ function options_field_storage_config_delete(FieldStorageConfigInterface $field_
+ * @throws \BadFunctionCallException
+ *

We should document when this is thrown.

Looks good apart from:

+++ b/core/modules/options/options.module
@@ -85,7 +87,12 @@ function options_allowed_values(FieldStorageDefinitionInterface $definition, Fie
+      if (is_callable($function, FALSE, $callable_name)) {

$callable_name is not defined.

Do we need to use the long form of is_callable() here anyway?

This should be postponed on #2982949: Introduce CallableResolver to help standardise the DX and error handling for callbacks across various subsystems.

Actually, I think this can be closed in favour of #2975503: allow FieldConfigInterface::setDefaultValueCallback() to accept a callback in service notation, which also depends on #2982949: Introduce CallableResolver to help standardise the DX and error handling for callbacks across various subsystems.

Static method notation works fine in versions of PHP that are now supported by Drupal 8. Eg:

      ->setSettings([
        'allowed_values_function' => [MyClass::class, 'myCallback'],
      ])

Per @joachim's comment in #42, looks like this issue should be closed as a duplicate in favor of #2975503: allow FieldConfigInterface::setDefaultValueCallback() to accept a callback in service notation. I'm closing this one and transferring credit over to that issue, as there was a lot of great discussion, review, patches, and triaging work here.

Triaged as part of the Bug Smash Initiative.

Since credit was transferred to the other issue #2975503: allow FieldConfigInterface::setDefaultValueCallback() to accept a callback in service notation, removing the credit here.