Convert function_exists() to is_callable()

@quietone Should we replace all existences of function_exists() with is_callable() ?

@annmarysruthy, I think each usage needs to be examined. For example, the usages in tests should probably not change, In may turn out that there are no places where making this change is an improvement. I have not looked closely at all the instances.

Checked all occurrences of function_exists() and changed 6 files. In my opinion, Rest of the occurrences are better kept as function_exists() itself. Kindly review MR

This looks great! I have to review more closely, but this already covers the three I was reviewing.

curious to compare performance of both methods

I'd take another look at EntityResolverManager.

Found some more hooks to exclude.

For 11.

Ok I am going through this and here are the criteria

I don't think we should be changing the string ones here. It's pointless and I think should be address in the issue that does the change.

I agree, that is the first bullet point, sorry if that wasn't clear.

If it is called on a string we know it's a function and we can address it when we convert the function itself

Ok I did the evaluation, it could use confirmation, I've attached it.

I ran this command to get a list of function_exists calls
grep -rn "function_exists" core > functionExistsCallsLine.txt

I then removed comments and broke it into three categories:

• Actionable (here or in a follow up)
• Procedural hooks, update, install, legacy invoke or theme related
• strings or library functions like is_file

Here is the list of actionable ones, I wasn't sure about the css test.
Category 1 Actionable
core/modules/views/src/Plugin/views/HandlerBase.php:572: if (isset($this->definition['access callback']) && function_exists($this->definition['access callback'])) {
core/includes/form.inc:955: if (($function = $batch['redirect_callback']) && function_exists($function)) {
core/modules/comment/src/CommentManager.php:210: if (function_exists($function)) {
core/modules/views_ui/src/ViewUI.php:341: $cancel_submit = function_exists($form_id . '_cancel') ? $form_id . '_cancel' : [$this, 'standardCancel'];
core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php:298:if (!function_exists('Drupal\Core\Asset\file_uri_scheme')) {
core/lib/Drupal/Core/Entity/EntityResolverManager.php:93: if (function_exists($controller)) {

I'm going to work on this.

Reducing scope here to just functions that have to be in a .module file.

nicxvan → changed the visibility of the branch 11.x to hidden.

I created a new MR removing the function_exists call in EntityManagerResolver so we can see what fails.

I accidentally pushed to the other branch so I force pushed that branch to reset it to the state annmarysruthy left it in.

Create follow up for CommentManager -> Needs a way to define the class and method.

📌 Deprecate magic ENTITY_TYPE_last_viewed functions Active

Create follow up for ViewsUI -> Needs a way to define the class and method.

📌 Deprecate magic cancel submit handlers in views ui Active

CssOptimizerUnitTest No need to address afaict

We can remove that whole extra namespace at the bottom of the test, since #3034072: Move file uri/scheme functions from file.inc and FileSystem to StreamWrapperManager → . Do you want another follow up or should it happen here?

EntityResolverManager does this just need deprecation? what situation can that be a function?

I think the failing test explains this one. I think it can stay, or update to is_callable.

Mixed feelings on this. I think almost all registered callbacks should use CallableResolver, for example the batch stuff, because then we could actually use services. I'm also not sure why we even check if for example the redirect_callback. If that's incorrectly defined then it just silently ignores it. As a developer, I'd much prefer to just get an exception/error on that.

For me the question is whether it's really worth changing to is_callable(), only to then either deprecate the whole thing or having it change it again. If we're going to deprecate something, we could just do it straight away? And for batch I'd prefer a dedicated issue to make everything use CallableResolver.

> I think the failing test explains this one. I think it can stay, or update to is_callable.

Well, it's a unit test testing the code, it was most likely written to explicitly cover every line of code. It's existence isn't necessarily proof that it's actually needed. I'd try removing the unit test to see if there's anything else. Unlike others, this is an explicit fallback. It can't be an array, and at this point, it can't really be anything else but a function. So an is_callable() check there seems pointless, either we keep it or we remove it..

Agree with @berdir. Using CallableResolver gives us more options and proper validation of callables.

Thank you everyone, that is exactly what I was looking for.

We should probably rename this since it's more about handling the last edge cases of functions required to be in .module files.

I'm ok with CallableResolver!

So in summary I think the form and handler we use CallableResolver.

In the test we just remove that namespace.

In the other cases we outright deprecate them in children issues.

This might need framework manager review.

I'm going to close @annmarysruthy's branch and hide it since we have a slightly different strategy now.

In my new branch I will use CallableResolver for form, and Handler.
I will look at updating the test and I will deprecate the function option for EntityResolver.

@berdir I not quite sure I follow this:

I'm also not sure why we even check if for example the redirect_callback. If that's incorrectly defined then it just silently ignores it. As a developer, I'd much prefer to just get an exception/error on that.

nicxvan → closed merge request !12733

nicxvan → closed merge request !11942

nicxvan → changed the visibility of the branch 3520416-testEntityResolverManager to hidden.

Ok I added CallbackResolver to form.inc, removed the extra namespace and deprecated that path in EntityResolverManager.

I'm sure I'll need to clean up tests.

Is there anything special to consider for injecting CallbackResolver in HandlerBase? I see it's using getters and setters for almost everything else, I'm not sure if there is a reason for that.