Problem/Motivation
According the the documentation we should be able to do the following:
tertiary:
type: string
title: Tertiary
# Limit the available options by using enums.
enum:
- success
- warning
- danger
# Provide a default value
default: success
However this doesn't seem to be implemented. In the tests this value is missing from the my-button component and is being passed in explicitly through the template instead.
Steps to reproduce
Failing test attached, see MR !5032 with the failing test report.
Proposed resolution
Implement the missing default functionality for SDC, see MR !5353, including both the test change, and the actual fix.
Remaining tasks
Maintainer review.
User interface changes
No.
API changes
No.
Documentation always suggested `default` should work for properties, so it is instead allowing that behavior.
Data model changes
No.
Release notes snippet
Maybe, hinting that default can be used now, as originally intended is a good idea, but may not be needed.
Also, let us change the related documentation to remove the "It isn't used in Twig template." around default, which would be implemented after this change gets in.
Comment | File | Size | Author |
---|---|---|---|
#13 | Screenshot_20240416_122134.png | 710.55 KB | e0ipso |
Issue fork drupal-3394815
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
justafishComment #6
wotnakAdded props default value functionality implementation in https://git.drupalcode.org/project/drupal/-/merge_requests/5353. It passes test modified by justafish.
I implemented it in `sdc_additional_context` twig function that is executed in ComponentNodeVisitor and already used to add things to component context. The implementation loops through component props and if a given prop has a default value set and doesn't already have a value in the current context, the default value is added to the context (which makes it available in the component template).
Comment #7
smustgrave CreditAttribution: smustgrave at Mobomo commentedCan the issue summary be updated please, left some TBD if they need to be filled in, can put NA if it doesn't apply to this issue.
Comment #8
FrankieD3Any updates on this functionality? If this has still yet to be implemented, the documentation should be altered to reflect the current state of the module so not to mislead developers.
Comment #9
marvil07 CreditAttribution: marvil07 at Tag1 Consulting commented@justafish, thanks for opening this issue, and providing a clear test change to reproduce it 👍
@wotnak, thanks for implementing a fix! 👍
Apart from the test fix, I also verified with another use-case on a custom theme that the default value is being used.
@smustgrave, I have updated the issue summary with a few more details.
Marking as RTBC.
Comment #13
e0ipsoThis has come up in the past in Slack. See this thread. Screenshooting for convenience:
In that thread @pdureau mentions:
I tend to agree. The JSON-Schema specification allows the
default
keyword, and we support JSON-Schema. However, I don't think the intention for it should be to inject data into the template. I think the correct use for it is to inform the application of a default value when building forms, or generating synthetic example renderings of a component, etc.While we could do what the IS seeks, we would face significant challenges down the road. Imagine when we need to translate the default value, or if the default value is a site URL and the site is installed in a subdirectory, or the default date should follow the site configured time zone, ...
In general, imagine that we need to use dynamically processed data as the default (which I think is a common use case). For that we need a place to declare the default with access to a runtime. The current recommendation is to use Twig to do so. This makes sense for me because:
content.field_foo|default('my default'|t)
.However, I acknowledge that there is confusion around this. The fact that it has come up more than once is an indicator. As brought up in #8.
I wonder if adding a section in the official FAQs is the way to fix this.
Comment #14
alexpottWe need to update https://www.drupal.org/docs/develop/theming-drupal/using-single-director... for sure as that's what as caused some of the confusion.
Comment #15
agentrickardI tend to agree with the assessment that this is a documentation/schema feature and not a means to inject data into Twig, which would make this a documentation issue.
I wonder if this part:
generating synthetic example renderings of a component
is something that might be implemented by modules like Storybook. If so, we could open a similar issue there.Comment #16
pdureau CreditAttribution: pdureau as a volunteer commented🙏
As a side note, using
default()
filter is a better practice than the collapsed (?:
) or null (??
) ternary operator:Comment #17
FrankieD3I feel as though the updated text "Provide a default value. It isn't used in Twig template." does not include enough context. If I were looking at this with fresh eyes, I would be thinking to myself, "then where is it being used?"
An example of a Twig template containing the use of the
default()
filter would be useful somewhere within the documentation. Something to the effect of{% set text = text|default('Placeholder'|t) %}
.