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.

CommentFileSizeAuthor
d8.state_.get-default.patch3.45 KBdamiankloip
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Issue summary: View changes

Updated issue summary.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

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

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Issue tags: +Quick fix

Tagging.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
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()?

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

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?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
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:

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

damiankloip CreditAttribution: damiankloip commented

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

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

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.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

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.

David Hernández’s picture

David Hernández CreditAttribution: David Hernández commented

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

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Assigned: Unassigned » alexpott

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

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +Configuration system
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: -Configuration system

Whoops, not this one.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

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).

amateescu’s picture

amateescu CreditAttribution: amateescu commented

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 :)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

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.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented

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.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
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.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented

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

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

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;
  }
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

I got convinced by that post to be honest!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Title: Add a default parameter to keyvalue get() method » Change 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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Issue summary: View changes

Updated issue summary.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Title: Change notice: Add a default parameter to keyvalue get() method » Add 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.