FEAT support variables as second and third argument #11521

lekoala · 2024-12-30T10:04:28Z

Description

This PR upgrades the text collector to support collecting arguments as variables for second and third argument (see examples below).

Manual testing steps

        // Should display last $str and support concatenation
        $str = 'Should NOT appear';
        $str = 'Lazy loading can increase perceived page performance by slightly delaying media loading. ' .
            'Eager loading will load files as soon as possible and can be used if the image is in view as the page ' .
            'loads (above or near the fold).';
        $titleTipContent = _t('DemoAdmin.LoadingTitleTip', $str);

        // Does not support variables, use {type}. This will improperly detect "test" as value for the translation
        $type = "test";
        $IDontHaveAValidSyntax = _t(__CLASS__ . '.CANNOT_DELETE', "Not allowed to delete '$type' records");

        // Support variable as third argument
        $var = ['some' => 'var'];
        $IWasNotCollected = _t('DemoAdmin.ImNOTCollected', 'test {some}', $var);

        // Support default case
        $ImCollected = _t('DemoAdmin.ImCollected', 'test {some}', ['some' => 'var']);

Issues

Pull request checklist

The target branch is correct
- See picking the right version
All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
- Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
The commit messages follow our commit message guidelines
The PR follows our contribution guidelines
Code changes follow our coding conventions
This change is covered with tests (or tests aren't necessary for this change)
Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
CI is green

GuySartorelli · 2025-01-05T23:37:17Z

Please fill in the PR checklist. That checklist is there to help you as the contributor double check you've done everything you need to do. That means that I as the reviewer will then know whether you have done those things which speeds things up a little.

GuySartorelli

Most of this review is just me asking for clarification, since I haven't worked in this part of the code much before so I need a bit of help understanding what's going on.

src/i18n/TextCollection/i18nTextCollector.php

GuySartorelli · 2025-01-05T23:43:28Z

src/i18n/TextCollection/i18nTextCollector.php

@@ -618,15 +618,31 @@ public function collectFromCode($content, $fileName, Module $module)
        $potentialClassName = null;
        $currentUse = null;
        $currentUseAlias = null;
+
+        $inVar = null;


What is "inVar"? Does this just track "something is in a variable"?
It looks like we use it to track only the most recently encountered variable in the entire file, but only until the assignment is finished being parsed and then we stop tracking it. Is that correct?
Might warrant an inline comment.

inVar is how i've collected $str = 'something' before the _t call is made
this allows $str = 'Lazy loading can increase perceived page performance by slightly delaying media loading. ' .
'Eager loading will load files as soon as possible and can be used if the image is in view as the page ' .
'loads (above or near the fold).';
$titleTipContent = _t('DemoAdmin.LoadingTitleTip', $str);
to work, even when being multiple strings concatenated together

Does it correctly work for adding to a variable later? e.g.

$x = 'some value'; $x .= ' and some more text';

src/i18n/TextCollection/i18nTextCollector.php

GuySartorelli · 2025-01-06T00:04:44Z

src/i18n/TextCollection/i18nTextCollector.php

+                        continue;
+                    }
+
+                    // The variable is the second argument or present in the parameter


What is "the parameter" in this context? Does that mean it's in the third (or subsequent) argument?

it's when you have _t('key', 'translation', $var) or _t('key', 'translation', 'comments' . $var) => not than this should be ever done anyway, but that can happen

So you mean it's the third argument (or after the second argument - I guess maybe you mean it could be in the third or later argument?)? If so please change the comment to specify that.

i'm not sure how to phrase that correctly :-)

src/i18n/TextCollection/i18nTextCollector.php

GuySartorelli · 2025-01-06T00:10:44Z

Please retarget this to the 5 branch - it's adding new functionality and has a risk of regression that I'd rather not introduce in a patch.

Co-authored-by: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com>

lekoala · 2025-01-06T09:58:10Z

@GuySartorelli ideally i'd like to make sure there is no regression
currently, code like this silverstripe/silverstripe-asset-admin#1532 is not being collected so I assume it is considered "valid code" according to current features, no?

GuySartorelli · 2025-01-08T20:24:41Z

Any change has some chance of regression. There's no known regressions this causes, but because it's code I'm unfamiliar with, there is a chance of regressions that I won't be able to identify.

Regardless, as this is adding new functionality, it is better included in a minor release rather than a patch release.

lekoala · 2025-01-09T16:46:37Z

ok rebased!

i don't know if any test or update is needed

a unit test would be nice but i didn't go as far as that. i've just tried locally on my project and it seemed to work fine

GuySartorelli · 2025-01-09T20:20:59Z

I won't have time to test this out myself until at least halfway through next week.
Unit tests would be great if you have time to implement them.

lekoala · 2025-01-10T13:06:33Z

@GuySartorelli i added the tests and improved how i track strings. i added some comments to make it more clear how it works. the whole test suite is running fine locally

src/i18n/TextCollection/i18nTextCollector.php

GuySartorelli · 2025-01-16T19:47:21Z

src/i18n/TextCollection/i18nTextCollector.php

@@ -733,6 +749,28 @@ public function collectFromCode($content, $fileName, Module $module)
                    continue;
                }

+                // Allow _t(Entity.Key, 'translation', $var) and expand _t(Entity.Key, $var)


Please respond to these questions.

src/i18n/TextCollection/i18nTextCollector.php

GuySartorelli · 2025-01-16T19:48:42Z

src/i18n/TextCollection/i18nTextCollector.php

+                        continue;
+                    }
+
+                    // The variable is the second argument or present in the parameter


So you mean it's the third argument (or after the second argument - I guess maybe you mean it could be in the third or later argument?)? If so please change the comment to specify that.

GuySartorelli · 2025-01-16T19:51:04Z

src/i18n/TextCollection/i18nTextCollector.php

@@ -618,15 +618,55 @@ public function collectFromCode($content, $fileName, Module $module)
        $potentialClassName = null;
        $currentUse = null;
        $currentUseAlias = null;
+        $inVar = null;


Suggested change

$inVar = null;

$inVar = null; // Tracks the value currently being assigned to a variable

As per #11521 (comment)
Have I understood that correctly? If not please adjust the comment so it's appropriately indicating the purpose of this variable.

it's not tracking the value, it's tracking the name of the variable. the value is stored in inVarText. i've added comments to reflect this

GuySartorelli · 2025-01-16T19:51:49Z

src/i18n/TextCollection/i18nTextCollector.php

@@ -618,15 +618,31 @@ public function collectFromCode($content, $fileName, Module $module)
        $potentialClassName = null;
        $currentUse = null;
        $currentUseAlias = null;
+
+        $inVar = null;


Does it correctly work for adding to a variable later? e.g.

$x = 'some value'; $x .= ' and some more text';

src/i18n/TextCollection/i18nTextCollector.php

Co-authored-by: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com>

lekoala · 2025-01-17T11:03:36Z

@GuySartorelli updated the code and added test cases for falsy values and $x = 'test' ; $x .= 'more';

FEAT support variables as second and third argument

0109080

lekoala mentioned this pull request Dec 30, 2024

FIX: Ensure _t() handles single-scalar and multi-scalar injections properly, and improve text collection logic #11520

Closed

8 tasks

revert invalid formatting

7da13fe

GuySartorelli requested changes Jan 6, 2025

View reviewed changes

Update src/i18n/TextCollection/i18nTextCollector.php

9f3ede8

Co-authored-by: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com>

GuySartorelli mentioned this pull request Jan 8, 2025

text collector doesn't collect when parameters are passed as a variable #11268

Open

2 tasks

lekoala changed the base branch from 5.3 to 5 January 9, 2025 16:43

lekoala added 3 commits January 10, 2025 13:58

add test for variables as second and third arguments

885a0ab

refactor string variables handling

e315c4c

move comment to docblock

73a64aa

no need to call processString again

1176da2

GuySartorelli requested changes Jan 16, 2025

View reviewed changes

lekoala and others added 6 commits January 17, 2025 10:45

update comment

8b80183

update comment

6342deb

explain inVar and inVarText

68454a1

Update src/i18n/TextCollection/i18nTextCollector.php

62cf6d6

Co-authored-by: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com>

add new test cases

8b253f8

support falsy values and .= operator

bd7115f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEAT support variables as second and third argument #11521

FEAT support variables as second and third argument #11521

lekoala commented Dec 30, 2024 •

edited

Loading

GuySartorelli commented Jan 5, 2025

GuySartorelli left a comment

GuySartorelli Jan 5, 2025

lekoala Jan 6, 2025

GuySartorelli Jan 16, 2025

GuySartorelli Jan 6, 2025

lekoala Jan 6, 2025

GuySartorelli Jan 16, 2025

lekoala Jan 17, 2025

GuySartorelli commented Jan 6, 2025

lekoala commented Jan 6, 2025

GuySartorelli commented Jan 8, 2025

lekoala commented Jan 9, 2025 •

edited

Loading

GuySartorelli commented Jan 9, 2025

lekoala commented Jan 10, 2025

GuySartorelli Jan 16, 2025

GuySartorelli Jan 16, 2025

GuySartorelli Jan 16, 2025

lekoala Jan 17, 2025

GuySartorelli Jan 16, 2025

lekoala commented Jan 17, 2025

	$inVar = null;
	$inVar = null; // Tracks the value currently being assigned to a variable

FEAT support variables as second and third argument #11521

Are you sure you want to change the base?

FEAT support variables as second and third argument #11521

Conversation

lekoala commented Dec 30, 2024 • edited Loading

Description

Manual testing steps

Issues

Pull request checklist

GuySartorelli commented Jan 5, 2025

GuySartorelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli commented Jan 6, 2025

lekoala commented Jan 6, 2025

GuySartorelli commented Jan 8, 2025

lekoala commented Jan 9, 2025 • edited Loading

GuySartorelli commented Jan 9, 2025

lekoala commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lekoala commented Jan 17, 2025

lekoala commented Dec 30, 2024 •

edited

Loading

lekoala commented Jan 9, 2025 •

edited

Loading