User tests: Successful: Unsuccessful:
While testing #20491 I found out that it would be very convenient to allow labels and descriptions strings to use a variable (%s) in order to simplify the multiple use of a value with a Text::sprintf
.
Added 2 new elements to FormField:
varLabel
and varDescription
Using sprintf when these elements have a value.
Also modified file to use Text::
instead of \JText::
Patch.
Modify an xml. For this example I just modified /administrator/components/com_content/forms/article.xml
<field
name="image_intro"
type="media"
label="COM_CONTENT_FIELD_INTRO_LABEL"
description="COM_CONTENT_FIELD_INTRO_DESCRIPTION"
varLabel="JYES"
varDescription="JNO"
/>
Then I modified and added a description string in /administrator/language/en-GB/en-GB.com_content.ini
line 53-54
COM_CONTENT_FIELD_INTRO_DESCRIPTION="Select an image %s"
COM_CONTENT_FIELD_INTRO_LABEL="Intro Image %s"
Remark that I added the placeholder for the variable %s
YES.
Welcome any corrections/suggestions for the comments.
At this stage, this PR allows only one variable.
It would be great if it was improved to allow many variables so that we can use %1$s
%2$s
etc.
Status | New | ⇒ | Pending |
Category | ⇒ | Libraries |
You mean varDescription and varLabel all over?
Yes.
Labels |
Added:
?
|
camelCase done.
I have tested this item
As you said, if this is accepted, we need extend to more variables
Since most descriptions have been removed from labels, let's only check $varDescription
when there is a description. $varDescription
can be 0
so check for empty string. Furthermore I would say there is no need for $varDescription
and $varLabel
. Here is my proposed code. The same can be done with $varLabel
.
// Description preprocess
$description = !empty($this->description) ? $this->description : null;
if (!empty($description) && $this->translateDescription)
{
// Do we have a text variable to use when displaying the description?
if ($this->varDescription !== '')
{
$description = Text::sprintf($description, $this->varDescription);
}
else
{
$description = Text::_($description);
}
}
Since Formfield was modified, I have to redo this patch anyway. Will also modify its code.
Category | Libraries | ⇒ | Installation Libraries |
Category | Libraries Installation | ⇒ | Libraries |
@Quy @SharkyKZ @carlitorweb
You now can test with the new code.
For me it is hard to find a use case for this pr, where you have two hard coded values in the XML definition to put into the label or description. Why not directly add them.
$varDescription
and $varLabel
are obsolete.
$varDescription and $varLabel are obsolete.
Hmm, why do we have $hint or $type then? (just for my knowledge).
I have tested this item
I have tested this item
Status | Pending | ⇒ | Ready to Commit |
RTC
Labels |
Added:
?
|
Drone restarted.
Restarted drone again... Failure is not due to this PR.
@Webdongle @laoneo Can you merge this?
@infograf768 I do not (and never have) had ability to merge on github. Also I have resigned from bugsquad.
Title |
|
Title |
|
it has been a year - will this be merged or not? Is it waiting on something
Not as it is. I don't want to have this forced single string. The concept is almost there but this needs to be able to take a list of items otherwise we're just going to hit massive limitations. Trying to sort this in a vaguely b/c way for form fields is super complicated tho :(
Status | Ready to Commit | ⇒ | Needs Review |
Just my 2cents, if you need variables in your language strings, you may as well set a new description in the code using something like $this->form->setFieldAttribute('description, 'a new string')
.
Other than that I would say we can close this as it can be solved in a different and more flexible way.
I have tested this item
This needs to be looked at again according to the comments made. Would be worth revisiting and taking in to account @george and @RolandS comments
@flo-the-cat tagged the wrong George. @wilsonge is this one still on your radar?
@infograf768 your thoughts on whether it's still needed in light of the conversation in 2019?
Or per @roland-d comment should it be closed.
Commenting on this PR as part of a "Outstanding PR Status List" exercise to work on reducing the number of outstanding PR that have been otherwise orphaned, neglected or forgotten instead of being merged. See Glip Bugs & Fun @ Home channel for more information.
I still think this would be a nice improvement to avoid wrong translation of some strings that should not be translated.
Read #20525 (comment)
I don't think @roland-d proposal solves the issue and fail to understand @wilsonge comment.
@infograf768 I think the issue is that the example you gave in the first post doesnt really show a good example of how this would be used. If you post an example using one of the com_csp forms I think people might understand the need for this more.
Example with label (in these cases, sprintf is not used.)
Modifying xml to add varLabel
in the 2 following cases in /administrator/components/com_csp/config.xml
<field
name="contentsecuritypolicy"
type="radio"
label="COM_CSP_CONTENTSECURITYPOLICY"
layout="joomla.form.field.radio.switcher"
default="0"
varLabel="href='https://scotthelme.co.uk/content-security-policy-an-introduction' target='_blank' rel='noopener noreferrer'>Content Security Policy (CSP)"
>
<option value="0">JDISABLED</option>
<option value="1">JENABLED</option>
</field>
[...]
<field
name="frame_ancestors_self_enabled"
type="radio"
label="COM_CSP_CONTENTSECURITYPOLICY_FRAME_ANCESTORS_SELF_ENABLED"
description="COM_CSP_CONTENTSECURITYPOLICY_FRAME_ANCESTORS_SELF_ENABLED_DESC"
layout="joomla.form.field.radio.switcher"
default="1"
showon="contentsecuritypolicy:1[AND]contentsecuritypolicy_mode!:detect"
varLabel="frame-ancestors 'self'"
>
<option value="0">JDISABLED</option>
<option value="1">JENABLED</option>
</field>
Original strings:
; Please do not translate the following language string
COM_CSP_CONTENTSECURITYPOLICY="<a href='https://scotthelme.co.uk/content-security-policy-an-introduction' target='_blank' rel='noopener noreferrer'>Content Security Policy (CSP)</a>"
; Please do not translate the following language string
COM_CSP_CONTENTSECURITYPOLICY_FRAME_ANCESTORS_SELF_ENABLED="frame-ancestors 'self'"
Modified strings:
COM_CSP_CONTENTSECURITYPOLICY="<a %s </a>"
COM_CSP_CONTENTSECURITYPOLICY_FRAME_ANCESTORS_SELF_ENABLED="%s"
For a string like
; Please do not translate 'max-age' in the following language string
PLG_SYSTEM_HTTPHEADERS_HSTS_MAXAGE_DESC="This option sets the time for 'max-age', it is specified in seconds. The default value is 31536000, which corresponds to one year"
the sprintf can be used with varDescription
and we would have
PLG_SYSTEM_HTTPHEADERS_HSTS_MAXAGE_DESC="This option sets the time for '%s', it is specified in seconds. The default value is 31536000, which corresponds to one year"
If this PR is improved to allow multiple variables, we could also simplify a string like
; Please do not translate 'Content-Security-Policy' & 'Content-Security-Policy-Report-Only' in the following language string
COM_CSP_CONTENTSECURITYPOLICY_REPORT_ONLY_DESC="Use the header 'Content-Security-Policy-Report-Only' instead of 'Content-Security-Policy'."
Thank you - I think people should be able to see the benefit of this now
I am moving this to 4.1, we are in beta and this is a new feature.
Title |
|
So this did not make it in 4.0 and from my point of view will not make it into 4.1 either, although I see the benefit of the whole idea the limitations stated by @wilsonge are still valid (especially that you can only add one variable...) and TBH for me it feels super complicated when strings are splitted up in several %s, too.
I think this could be reconsidered, when we would go for "custom named placeholders" to make language keys more clear. (See: https://github.com/joomla/joomla-cms/blob/4.1-dev/libraries/src/Language/Text.php#L113-L122)
Status | Needs Review | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-01-20 03:22:12 |
Closed_By | ⇒ | bembelimen | |
Labels |
Added:
?
?
Removed: ? ? |
Please use camelCase where applicable.