-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Leverage FieldDescriptionInterface::getOption()
in views
#7038
Leverage FieldDescriptionInterface::getOption()
in views
#7038
Conversation
I think that could fix: #7029 |
e8a89a4
to
8c0cc9e
Compare
8c0cc9e
to
bd28d86
Compare
})) }} | ||
{% elseif sonata_admin.field_description.options.placeholder is defined and sonata_admin.field_description.options.placeholder %} | ||
{% elseif sonata_admin.field_description.option('placeholder') %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a general rule, should we change from:
variable.field_description.options.some_option is defined and variable.field_description.options.some_option
to
variable.field_description.option('some_option') is not null
to be explicit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, because variable.field_description.option('some_option') is not null
evaluates to true
when the "some_option" option is set with the value false
, and in this example, the condition is trying to check if the option is holding a truthy value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should add a hasOption
method, that would make things clearer IMO (but that's another issue).
@phansys This is a BC-break Before, using
in a non-fielddescriptions context could work, since there was a Also, before passgin I think it could be great having the
And the code
would be changed to
The current bugfix could be
The BC-way to introduce the feature could be
WDYT ? |
I've experiencing the same issue with one of my custom views, but I dismissed the problem since I think we haven't a BC promise around our internal templates. |
I meant Inverse|default(fielddescription.option(inverse))|default(false) |
LGTM. |
Subject
Leverage
FieldDescriptionInterface::getOption()
in views.I am targeting this branch, because these changes respect BC.
Many usages of the Twig's
default
filter were removed, leveraging the argument 2 ofFieldDescriptionInterface::getOption()
.Changelog