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.
Related Issues
Comment | File | Size | Author |
---|---|---|---|
d8.state_.get-default.patch | 3.45 KB | damiankloip | |
Comments
Comment #0.0
damiankloip CreditAttribution: damiankloip commentedUpdated issue summary.
Comment #1
dawehnerSimple enough. This makes life for peope easier so +1
Comment #2
jibranTagging.
Comment #3
webchickHm, 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()?
Comment #4
damiankloip CreditAttribution: damiankloip commentedI 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?
Comment #5
dawehnerYeah 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:
Comment #6
damiankloip CreditAttribution: damiankloip commentedYep, 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
Comment #7
webchickBut 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.vs.
I guess I'll leave it for another core committer to evaluate but I'm not really +1 on this, myself.
Comment #8
damiankloip CreditAttribution: damiankloip commentedWe 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.
Comment #9
David Hernández CreditAttribution: David Hernández commentedd8.state_.get-default.patch queued for re-testing.
Comment #10
webchickStill don't like this idea (see #7 for rationale), but I'll ask alexpott to make the call.
Comment #11
xjmComment #12
xjmWhoops, not this one.
Comment #13
damiankloip CreditAttribution: damiankloip commentedSo 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).
Comment #14
amateescu CreditAttribution: amateescu commentedI 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 :)
Comment #15
dawehnerWhile 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.
Comment #16
ianthomas_ukWhatever 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.
Comment #17
tim.plunkettPlease 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.
Comment #18
ianthomas_uk@tim.plunkett: I've reopened #2090751: Add a default parameter to the config get() method for config().
Comment #19
damiankloip CreditAttribution: damiankloip commentedYes, 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 doisset($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:
Comment #20
dawehnerI got convinced by that post to be honest!
Comment #21
alexpottI spoke to @msonnabaum on IRC as the original coder of the key value store and he said:
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
Comment #21.0
alexpottUpdated issue summary.
Comment #22
star-szrThe 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