FEAT support variables as second and third argument

[lekoala]

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

• text collector doesn't collect when parameters are passed as a variable #11268
• LoadingTitleTip is not collected silverstripe-asset-admin#1532

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]

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.

[GuySartorelli]

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.

[lekoala]

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

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]

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]

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]

@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

[lekoala]

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

[GuySartorelli]

Looks good, great unit test. Thank you!