Silverstripe-framework: [I18N] BUG i18nTextCollector should support SomeClass::class syntax

[robbieaverill]

Currently the following would cause an error in the text collector:

return _t(SomeClass::class . '.Title', 'Some title');

As a result, all modules need to use FCQN in strings when referencing translations in other files.

We should fix the parser to support ::class syntax.

For reference, __CLASS__ currently works.

Pull requests

• [x] Support for self::class: #7648
• [ ] Support for Some::class: TBC

[unclecheese]

Just having a quick look -- it appears as if the intention is to skip entities that cannot be statically analysed. https://github.com/silverstripe/silverstripe-framework/blob/4.0/src/i18n/TextCollection/i18nTextCollector.php#L619

What was the error you were getting?

[robbieaverill]

I've added a PR to enable self::class at #7648.

As @unclecheese noted, due to the fact that this is using static code analysis, it's not realistic to allow parent::class (without reflection) or static::class at this stage.

I've got a WIP to enable SomeObject::class support, which can be done with the following assumptions:

• SomeObject has been imported via a use statement, or;
• SomeObject exists in the same namespace (global or otherwise) as the current class

I think with the two assumptions above that the first one is safe, because the class has a use for the class, so it's not that important if it exists. The second assumption is "safe" in that it's how PHP would treat the class too.

I'll chip away at this for a few days - I facepalmed pretty hard when I looked at this code.

[robbieaverill]

Yeah @unclecheese - there is some code that's using ::class at the moment (it's already been noted that it's an issue, although not actioned yet) - see SiteConfigLeftAndMain for an example which has a set of LeftAndMain::class calls in it.

I definitely feel like using an AST parser with traversal would be a huge benefit here, but it would be more of a structural replacement than a "fix this ticket" change.

[unclecheese]

Nice work on this, @robbieaverill .

[tractorcow]

Instead of implementing this ourselves we should use the nikic/php-parser that's now a part of core.

In addition, I'd not be against splitting text collector into it's own module.

[robbieaverill]

In addition, I'd not be against splitting text collector into it's own module.

That seems like a good idea to me, particularly because it's mostly used by module devs rather than project devs

[robbieaverill]

It sounds like @NightJar is keen to get involved with the remaining work on this too =)

[lekoala]

any update on this? SiteConfig uses SiteTree::class syntax which is not supported. Which, as far as I can tell, prevent using the text collector at all because of the call to getEntitiesByModule which parse all modules anyway.

[robbieaverill]

I don’t think it prevents text collection, but it certainly fills up your console with plenty of errors. Siteconfig and other modules are still incorrectly written and we would appreciate any pull requests to fix them up.

I got self::class going but SomeClass::class involves factoring in namespaces and is a bit trickier without using a php parser in this code. I got sidetracked and forgot about this :-D

[NightJar]

Not that I'm aware of sorry. I desired to get amongst it, but never found the time.

[lekoala]

I have thought a little about this, and maybe it's not quite necessary to support this syntax in _t. The current behaviour force the TextCollector to look at all modules to determine the "master" string. This adds a lot of complexity.
A simpler approach would be to have a clear distinction between "local" translation (inside the module) and "external" translation (use a translation from another module)

The whole thing could be then really simple:

_t('Entity.KEY', 'Default translation');
_g('Entity.KEY'); // here I introduce a _g helper which is for "global translation" but it could be _t('ext:Entity.KEY') for instance

Also, I believe we could skip the entity entirely and just do

_t('KEY', 'Default translation') ; // in that scenario, Entity would be the filename without the extension which would enforce consistent naming without any cost or __CLASS__ or static::class or whatever

This approach remove a lot of complexity : no need to "collect" or "handle" external translation. No need to provide a default translation when using an external translation (the current setting forces us to provide a translation otherwise we have a warning). Text collecting also become a lot easier because each module can be collected on its own.

I'm just saying that because I'm building my own TextCollector to overcome these specific issues, and it might be worth considering adding that to the core.

[robbieaverill]

I'm having a look at this now, and will continue on Friday. I started by using nikic/php-parser as an AST parser, which handles FQCN strings and SomeClass::class namespace resolution nicely, but since it works at parse time it still won't handle constants like __CLASS__. We can track the current class name and replace __CLASS__ and self::class with it, but that still won't work for parent::class and static::class (even though they're edge case). I'm going to have a look into using phpstan instead, or a combination of the two. Will update when I have some findings =)

[robbieaverill]

I've got an MVP module for collecting i18n entities from PHP classes using an AST parser: https://github.com/robbieaverill/silverstripe-textcollector. This is still a parse time approach, so won't support parent::class, static::class, or variables in keys/values, but now supports SomeClass::class easily. I haven't put much work into how to fold it into framework other than a quick patch to plug it in.