This is an extension of #1875086: Improve DX of drupal_container()->get(), which leaves us with deprecated, inefficient, and clumsy code like

  if (Drupal::getContainer()->has($context_name)) {
    $context = Drupal::service($context_name);
    // ...
  }

or having to catch exceptions as part of our normal program flow.

I'd like to have something like this instead:

  if ($context = Drupal::service($context_name, TRUE)) {
    // ...
  }

Symphony's \Symfony\Component\DependencyInjection\ContainerInterface already has the option to call get() with NULL_ON_INVALID_REFERENCE rather than the default EXCEPTION_ON_INVALID_REFERENCE, so this is easy to implement.

In image.inc we currently have this:

function image_get_info($filepath, ImageToolkitInterface $toolkit = NULL) {
  $details = FALSE;
  if (!is_file($filepath) && !is_uploaded_file($filepath)) {
    return $details;
  }

  if ($toolkit === NULL) {
    $toolkit = Drupal::service('image.toolkit');     // <=== can throw!
  }
  if ($toolkit) {
    $image = new stdClass();
    $image->source = $filepath;
    $image->toolkit = $toolkit;
    $details = $toolkit->getInfo($image);
    if (isset($details) && is_array($details)) {
      $details['file_size'] = filesize($filepath);
    }
  }

  return $details;
}

... which clearly assumes nothrow behavior. With the following line it will work as expected:

    $toolkit = Drupal::service('image.toolkit', TRUE);
CommentFileSizeAuthor
#3 nothrow_hint.png3.77 KBsalvis
#1 Drupal_service_nothrow.1957804.1.patch1.19 KBsalvis
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented
Status: Active » Needs review
FileSize
Drupal_service_nothrow.1957804.1.patch1.19 KB

This patch adds the $nothrow parameter and fixes image_get_info().

amateescu’s picture

amateescu CreditAttribution: amateescu commented

ContainerInterface also supports IGNORE_ON_INVALID_REFERENCE, so how about just adding a second parameter ($invalidBehavior) to Drupal::service?

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented
FileSize
nothrow_hint.png3.77 KB

I prefer typing

$toolkit = Drupal::service('image.toolkit', TRUE);

over

$toolkit = Drupal::service('image.toolkit', ContainerInterface::NULL_ON_INVALID_REFERENCE);

PhpStorm offers me a clean and succinct hint:

screenshot

amateescu’s picture

amateescu CreditAttribution: amateescu commented

Ok, but what to do with IGNORE_ON_INVALID_REFERENCE then? Restrict people's acces to that option? On what basis? :)

yched’s picture

yched CreditAttribution: yched commented

I'd also think we want to use the existing Symfony constants here rather than reinventing boolean params on top of them.

A bit more tedious to write, but much more easier to read & mentally parse. Boolean flags don't mean anything when you look at existing code, unless you open the function definition.

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented

Ok, but what to do with IGNORE_ON_INVALID_REFERENCE then? Restrict people's acces to that option?

I'm not restricting anything. That option is not available at this point, and I don't see any need to make it available.

What is your use case for IGNORE_ON_INVALID_REFERENCE?

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented

BTW, this will allow us to rewrite

  if (drupal_container()->has('kernel')) {
    $kernel = drupal_container()->get('kernel');
  }
  else {
    // Immediately boot a kernel to have real services ready.
    $kernel = new DrupalKernel('install', FALSE, drupal_classloader(), FALSE);
    $kernel->boot();
  }

in install.inc as

  if (!$kernel = Drupal::service('kernel', TRUE))
    // Immediately boot a kernel to have real services ready.
    $kernel = new DrupalKernel('install', FALSE, drupal_classloader(), FALSE);
    $kernel->boot();
  }

rather than having to mess with exceptions. Wouldn't it be nice to have some code actually get slimmer?

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented

@yched: I think the usage (see #7above) is pretty clear from the context, without having to open the function definition.

An alternative would be to provide a second function serviceOrNull(), but I favor the bool parameter.

amateescu’s picture

amateescu CreditAttribution: amateescu commented

Re #8: The context doesn't give you *anything*, just that a service might be returned or not. I fail to see how a 'TRUE' parameter is supposed to tell a user that an exception will not be be thrown if the service is not found, as opposed to the longer version, ContainerInterface's constants, which are actually pretty clear when you see them.

I'm not restricting anything. That option is not available at this point, and I don't see any need to make it available.

Neither is the option for returning NULL, but you're adding it right now, and *that* means you are restricting the third option. If you don't see the need for it, that doesn't mean someone else won't need it. If that was the case, Symfony wouldn't have provided it in the first place?

Crell’s picture

Crell CreditAttribution: Crell commented

Agreed with yched in #5. Boolean params are not self-documenting. The Symfony constants are, make it clear what you're doing, and allow for more flexibility than just the boolean. If we're going to make Drupal:: a viable tool, then we should make it a viable tool right.

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented

Let's step back for a moment: IGNORE_ON_INVALID_REFERENCE never ever makes sense for get(), and trying to pass it would be a bug with unforeseeable consequences.

Looking at the Symphony code, Container::get() would exit without returning anything, essentially falling back to NULL_ON_INVALID_REFERENCE behavior (in an undocumented way), but we cannot know what some other ContainerInterface implementation might do in this situation.

If you really feel the need to make this ultra-self-explanatory, then we cannot have a default value and thousands of developers will have to type (and read) either ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE or ContainerInterface::NULL_ON_INVALID_REFERENCE again and again — or at least significant portions of either, because the editor cannot infer them from the function definition.

The overwhelming portion of Drupal::service() calls will rely on the service always being there, and it would be unreasonable to burden everyone with passing these constants, or even with checking for the exception or for NULL, for every call.

Viable is great, but fool-proof? I hate to be responsible for such a nuisance, because I started this thread...

Crell’s picture

Crell CreditAttribution: Crell commented

Nothing we've said precludes having a sane default. We should have a sane default. But constants are more self-documenting than booleans, and what we're specifying here isn't a boolean: It's not a true false, it's a "which method for error handling marker". It's not true/false, so should not be a boolean. (Just because something has 2 values doesn't make it a boolean.)

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented

When you read the code, how would you know what the default is without opening the function?

The author and reviewers of image_get_info() obviously had a different default in their mind.

I'm torn between the two views, but above all I hate having 45-character names... If you're willing to make the big step and compromise to have a default, it seems to be only a tiny further step to do without the overlong constant names and have mercy on those who'd have to deal with them.

<rant>
Sorry about insisting, but I'm frustrated with how discoverability has suffered and how all sorts of magic names and obscure strings need to be used in many places (for example to pass to Drupal::service(), which in turn returns a completely opaque object!), and OTOH we ask people to type 45 characters to make it look as if PHP had consistent type checking.

We've lost our sense of proportion. Asking developers to memorize and type things like ContainerInterface::NULL_ON_INVALID_REFERENCE is, well, a good way to scare them away. Why should you need to know that there's a \Symfony\Component\DependencyInjection\ContainerInterface behind Drupal::service()? Why would Drupal::service() leak this implementation detail?

I'd rather define Drupal::service($id, $invalidBehavior = 'throw') and tell people to pass 'nothrow' or 'null' or whatever they like, except 'throw', if they don't want 'throw'.
It's funny, this actually works with Symphony's Container::get() — pass 'nothrow' and it'll do what I want...
;-)
</rant>

adamdicarlo’s picture

adamdicarlo CreditAttribution: adamdicarlo commented

Agreed with @Crell: Constants are more self-documenting than booleans. Boolean function parameters are specifically called out to be avoided by books like Writing Solid Code and (IIRC) Code Complete.

Agreed with @salvis, too, though: Strings are fine here, as this is PHP. And, leaking that implementation detail (namespaced constant) makes no sense.

This vaguely reminds me of the few silly contrib modules I've seen that define constants for their permissions:

define('FOO_MODULE_BAR_BAZ_PERM_NAME', 'administer foo bar baz');

(Yech.)

Crell’s picture

Crell CreditAttribution: Crell commented

How is it leaking an implementation detail to forward the API call to the container, which is exactly what you're doing? This is a trivial wrapper around the DIC. The constant is part of the API for the DIC. If you want non-default behavior from the DIC, uh, use the DIC's API.

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented

How is it leaking an implementation detail to forward the API call to the container

The fact that Drupal::service() internally uses a \Symfony\Component\DependencyInjection\ContainerInterface is an implementation detail of the Drupal class. Right?

The caller of the current Drupal::service() API does not need to know this implementation detail in order to use the function effectively, right?

If we introduce a parameter that expects to get a ContainerInterface::NULL_ON_INVALID_REFERENCE value, the caller suddenly needs to know about ContainerInterface, right?

Information that used to be internal to the Drupal class has now leaked out into the world, and we have thus violated the principle of Information Hiding, right?

The constant is part of the API for the DIC. If you want non-default behavior from the DIC, uh, use the DIC's API.

You, Crell, know that this is Symphony's DIC implementation. But the caller of Drupal::service() should not need to know or care. A good wrapper wraps its internals and does not expose them.

Crell’s picture

Crell CreditAttribution: Crell commented

salvis: In general you'd be correct. In this case, however, if you don't know that Drupal:: is a legacy-support wrapper around the container object, you have no business using it. The container is an architectural lynchpin of Drupal 8; not knowing about it would be like not knowing about nodes. :-)

\Drupal is not an abstraction. It's a legacy shim. Thus knowing what you're shimming is a reasonable requirement.

ddrozdik’s picture

ddrozdik CreditAttribution: ddrozdik commented

Some work in this direction has already been made here #2001206: Replace drupal_container() with Drupal::service()

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented

#1: Drupal_service_nothrow.1957804.1.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, Drupal_service_nothrow.1957804.1.patch, failed testing.

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented
Status: Needs work » Closed (won't fix)

I get the impression that I'm of the type who "has no business using this," so let's leave it alone...