Updated: Comment #N

Problem/Motivation

At the moment we have to do things like $this->state->get('drupal_css_cache_files') ?: array(); and $this->state->get('language_count') ?: 1 because we can;t return a default value if no key is found.

This is what we could do with variable_get() calls before, Settings has it, and with symfony ParameterBags you can also do the same.

Proposed resolution

Add a $default parameter to KeyValueStoreInterface::get() method. This will allow us to do things like $this->state->get('drupal_css_cache_files', array()) and $this->$state->get('language_count', 1)

Remaining tasks

Patch, review, etc..

User interface changes

None

API changes

Optional $default parameter on state->get() calls for example.

Files: 
CommentFileSizeAuthor
d8.state_.get-default.patch3.45 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 59,262 pass(es).
[ View ]

Comments

Issue summary:View changes

Updated issue summary.

Status:Needs review» Reviewed & tested by the community

Simple enough. This makes life for peope easier so +1

Issue tags:+Quick fix

Tagging.

Status:Reviewed & tested by the community» Needs review

Hm, I'm not sure. Why add this to the state system, but not also the config system? And even if we did that, why do these arbitrary get() functions have a $default parameter but not also $container->get() or $entity->get()?

I have plans to add this anywhere else that makes sense, but tbh state makes the most sense, as this is the most likely to NOT find a value. Let's be honest, from the summary it makes code much less tedious.

I would like to add the same to the config system, some may argue you would always have default values stored, but I don't think this is always true. I will open a follow up. As for the others:

- $container->get() doesn't makes any sense, providing a default in place of a service?
- $entity->get(), not sure how much this makes sense in reality, but currently there is a todo in Entity to replace the get implementation when all entities have been converted. So maybe this could be added then?

Status:Needs review» Reviewed & tested by the community

Yeah that is exactly they point. The state system entries are created on the fly and never statically in code (I consider config as being in "code")
so you also have to care about a proper value:

There are many code snippets in code like:

<?php
  $deleted_fields
= $state->get('field.field.deleted') ?: array();
//...
 
$locale_plurals = \Drupal::state()->get('locale.translation.plurals') ?: array();
?>

Yep, thank you. snippets like that (and ones like in the issue summary) are everywhere, you almost need it for all your state calls.

Created the config equivalent (not as much as state, but even the logic in get() suggested a default value should be added): #2090751: Add a default parameter to the config get() method

But I guess my point is $foo = something() ?: array() is a standard PHP 5.3+ pattern. People are going to see it anywhere you say "If this thing has a value in it, assign it to $foo. Otherwise, give it an empty array." So making it so sometimes they don't see that pattern, because the second parameter of a function is actually the default value (which you don't know without looking at the signature for that function) is strange to me. It doesn't even really save any typing.

$deleted_fields = $state->get('field.field.deleted') ?: array(); # 65 characters

vs.

$deleted_fields = $state->get('field.field.deleted', array()); # 62 characters

I guess I'll leave it for another core committer to evaluate but I'm not really +1 on this, myself.

We currently use the same pattern for Settings(), and have used this for variable_get(), drupal_static(), element_info_property() (Which are still around). So having a default parameter is not a completely foreign concept.

Also, if you don't want to use the default pattern, just use the ternary shorthand you described above, things would be no different if you still wanted do that :)

EDIT: It also seems unnecessary wrapping (what's basically a ternary) in another ternary when we don't need to.

d8.state_.get-default.patch queued for re-testing.

Assigned:Unassigned» alexpott

Still don't like this idea (see #7 for rationale), but I'll ask alexpott to make the call.

Issue tags:+Configuration system

Issue tags:-Configuration system

Whoops, not this one.

So for anyone interested, examples in Symfony:

- Symfony\Component\HttpFoundation\Session\Session
- Symfony\Component\HttpFoundation\Session\Attribute\AttributeBag

So, further to #9, state values are by no means guaranteed to be there. So having a default param kind of implies this a bit better too. Plus I don't see a need to wrap a ternary in another ternary all the time :p (You can still use that pattern though, as this parameter is totally optional).

I was on the fence with this proposal because I thought that providing a default was mostly a drupalism, but apparently it's not, so I think the change makes sense :)

While I do agree that having a default value on state makes conceptually sense I am not convinced about a generic KV store,
as you push and pull values from there.

Whatever the decision on this is, I think it should apply to both state and config to save your average developer needing to regularly look up which is which, especially because if you provide a second parameter that isn't recognised as a default by then you'll just get null.

I was about to come out in favour of the :? syntax (on the basis that it was more consistent with other PHP code and a performance advantage if you need to calculate the default), but then realised that it doesn't actually do the same thing as D7's variable_get:

:? tests using empty(), but we should be testing using isset().

In most cases this won't matter, but it is very important if you've got a boolean with a default of true.

Status:Reviewed & tested by the community» Needs review

Please leave config() out of this. If someone wants to open an issue to discuss that, I can explain why it's a won't fix.

As far as this goes, I think #15 is right. We should consider this for state, but pushing it all the way up to all KV stores is a bit much.

@tim.plunkett: I've reopened #2090751: Add a default parameter to the config get() method for config().

Yes, let's leave config out of this. That was opened purely to make it consistent with this issue, but everyone agrees it does not really make sense.

So the empty() checking in the ternary is a good point. In reality the pretty much means people will not be able to realise their dream of using $state->get('blah') ?: 'blah' anyway, as really you will certainly need to do isset($state->get('blah')) ? $state->get('blah') : 'blah' (in some cases even array_key_exists(), who knows) - doesn't look so cool, which pretty much leans back to making the default value option of this patch much more appealing, no?

Also, we are implicitly giving people a default value anyway:

  public function get($key) {
    $values = $this->getMultiple(array($key));
    return isset($values[$key]) ? $values[$key] : NULL;
  }

Status:Needs review» Reviewed & tested by the community

I got convinced by that post to be honest!

Title:Add a default parameter to keyvalue get() methodChange notice: Add a default parameter to keyvalue get() method
Priority:Normal» Major
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

I spoke to @msonnabaum on IRC as the original coder of the key value store and he said:

17:27 msonnabaum: alexpott: yeah, I'm fine with it I think. Although I don't really find the ?: syntax problematic

So on that basis and the reasoning in #19 I'm going to commit.

Committed 4a3ee31 and pushed to 8.x. Thanks!

Need to update https://drupal.org/node/1790518

Issue summary:View changes

Updated issue summary.

Title:Change notice: Add a default parameter to keyvalue get() methodAdd a default parameter to keyvalue get() method
Assigned:alexpott» Unassigned
Priority:Major» Normal
Issue summary:View changes
Status:Active» Fixed
Issue tags:-Needs change record

The update to the change record was made in October by @damiankloip (thanks!), I think we can close this issue.

https://drupal.org/node/1790518/revisions/view/2726123/2884279

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.