? ? Pending

User tests: Successful: Unsuccessful:

avatar infograf768
infograf768
22 May 2018

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.

Summary of Changes

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

Testing Instructions

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

Result

screen shot 2018-05-22 at 08 35 18

Documentation Changes Required

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.

avatar infograf768 infograf768 - open - 22 May 2018
avatar infograf768 infograf768 - change - 22 May 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 22 May 2018
Category Libraries
avatar SharkyKZ
SharkyKZ - comment - 22 May 2018

Please use camelCase where applicable.

avatar infograf768
infograf768 - comment - 22 May 2018

You mean varDescription and varLabel all over?

avatar SharkyKZ
SharkyKZ - comment - 22 May 2018

Yes.

avatar infograf768 infograf768 - change - 22 May 2018
Labels Added: ?
avatar infograf768
infograf768 - comment - 22 May 2018

camelCase done.

avatar infograf768 infograf768 - change - 22 May 2018
The description was changed
avatar infograf768 infograf768 - edited - 22 May 2018
avatar carlitorweb
carlitorweb - comment - 22 May 2018

I have tested this item successfully on b0f943a

As you said, if this is accepted, we need extend to more variables


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20525.

avatar carlitorweb carlitorweb - test_item - 22 May 2018 - Tested successfully
avatar Quy
Quy - comment - 22 May 2018

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);
                        }
                }
avatar infograf768
infograf768 - comment - 23 May 2018

Since Formfield was modified, I have to redo this patch anyway. Will also modify its code.

avatar joomla-cms-bot joomla-cms-bot - change - 23 May 2018
Category Libraries Installation Libraries
avatar joomla-cms-bot joomla-cms-bot - change - 23 May 2018
Category Libraries Installation Libraries
avatar infograf768
infograf768 - comment - 23 May 2018

@Quy @SharkyKZ @carlitorweb
You now can test with the new code.

avatar laoneo
laoneo - comment - 23 May 2018

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.

avatar brianteeman
brianteeman - comment - 23 May 2018

@laoneo the specific use case would be for a plugin like system http headers where we have a large number of terms that should never be translated and it is currently hard to notify tt to ignore those terms

avatar Quy
Quy - comment - 23 May 2018

$varDescription and $varLabel are obsolete.

avatar infograf768
infograf768 - comment - 23 May 2018

$varDescription and $varLabel are obsolete.

Hmm, why do we have $hint or $type then? (just for my knowledge).

avatar infograf768 infograf768 - change - 24 May 2018
The description was changed
avatar infograf768 infograf768 - edited - 24 May 2018
avatar infograf768 infograf768 - change - 24 May 2018
The description was changed
avatar infograf768 infograf768 - edited - 24 May 2018
avatar Quy
Quy - comment - 26 May 2018

I have tested this item successfully on 26059d8


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20525.

avatar Quy Quy - test_item - 26 May 2018 - Tested successfully
avatar jreys
jreys - comment - 28 May 2018

I have tested this item successfully on 26059d8


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20525.

avatar jreys jreys - test_item - 28 May 2018 - Tested successfully
avatar Quy Quy - change - 28 May 2018
Status Pending Ready to Commit
avatar Quy
Quy - comment - 28 May 2018

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20525.

avatar infograf768 infograf768 - change - 10 Jun 2018
Labels Added: ?
avatar infograf768
infograf768 - comment - 15 Jul 2018

Drone restarted.

avatar infograf768
infograf768 - comment - 15 Jul 2018

Restarted drone again... Failure is not due to this PR.
@Webdongle @laoneo Can you merge this?

avatar Webdongle
Webdongle - comment - 15 Jul 2018

@infograf768 I do not (and never have) had ability to merge on github. Also I have resigned from bugsquad.

avatar infograf768
infograf768 - comment - 15 Jul 2018

oops, sorry. I meant @wilsonge ?

avatar franz-wohlkoenig franz-wohlkoenig - change - 19 Apr 2019
Title
[4.0] New feature: Allowing adding a variable for the field Label and Description elements
[4.0] Allowing adding a variable for the field Label and Description elements
avatar franz-wohlkoenig franz-wohlkoenig - edited - 19 Apr 2019
avatar franz-wohlkoenig franz-wohlkoenig - change - 19 Apr 2019
Title
[4.0] New feature: Allowing adding a variable for the field Label and Description elements
[4.0] Allowing adding a variable for the field Label and Description elements
avatar brianteeman
brianteeman - comment - 19 Oct 2019

it has been a year - will this be merged or not? Is it waiting on something

avatar wilsonge
wilsonge - comment - 25 Oct 2019

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

avatar wilsonge wilsonge - change - 30 Nov 2019
Status Ready to Commit Needs Review
avatar roland-d
roland-d - comment - 1 Aug 2020

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.

avatar flo-the-cat flo-the-cat - test_item - 16 Oct 2020 - Tested unsuccessfully
avatar flo-the-cat
flo-the-cat - comment - 16 Oct 2020

I have tested this item ? unsuccessfully on 6c96029

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


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20525.

avatar particthistle
particthistle - comment - 15 Jan 2021

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

avatar infograf768
infograf768 - comment - 15 Jan 2021

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.

avatar brianteeman
brianteeman - comment - 15 Jan 2021

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

avatar infograf768
infograf768 - comment - 15 Jan 2021

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"

Result
Screen Shot 2021-01-15 at 11 08 38

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'."
avatar brianteeman
brianteeman - comment - 15 Jan 2021

Thank you - I think people should be able to see the benefit of this now

avatar rdeutz
rdeutz - comment - 15 Mar 2021

I am moving this to 4.1, we are in beta and this is a new feature.

avatar rdeutz rdeutz - change - 15 Mar 2021
Title
[4.0] Allowing adding a variable for the field Label and Description elements
[4.1] Allowing adding a variable for the field Label and Description elements
avatar rdeutz rdeutz - edited - 15 Mar 2021
avatar bembelimen
bembelimen - comment - 20 Jan 2022

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)

avatar bembelimen bembelimen - close - 20 Jan 2022
avatar bembelimen bembelimen - change - 20 Jan 2022
Status Needs Review Closed
Closed_Date 0000-00-00 00:00:00 2022-01-20 03:22:12
Closed_By bembelimen
Labels Added: ? ?
Removed: ? ?

Add a Comment

Login with GitHub to post a comment