Don't make ParsedFunction final (or introduce a couple of additional getter/setter)

[drzraf]

It would be handy to define a parsedFunction in two steps. And particularly to change it's name after some interpolation and arguments identification has been done. (Sample use-case: I've two functions mapping to dngettext)

Scanner pseudo-code:

public function scan(string $code, string $filename): array
    $found_functions = $this->foo();
    // Now create parsedFunctions[]
    foreach ($found_functions as $function) {
        list($name, $line, $args) = $function;
        $p = new ParsedFunction($name, $filename, $line); // factor this line of code since it always happens

        switch ($name) { // prepare with parsedFunction **arguments**
            // ... but may need to actually change ParsedFunction's $name
        }
    }
}

[oscarotero]

(Sample use-case: I've two functions mapping to dngettext)

Not sure if I understand what you mean. You can have two different functions names mapping to the same gettext function. See this code: https://github.com/php-gettext/Gettext/blob/master/src/Scanner/CodeScanner.php#L19

There are many functions (gettext, __, _, noop, noop__) mapped to the same gettext.

[drzraf]

But https://developer.wordpress.org/reference/functions/__/ could map to either gettext or dgettext (if $domain is null). As such I've to handle the optional presence of the domain parameter and having a setName() method would provide useful.

Making this class final keeps an implementer from extending with a getDomain()-like function. I can't at the same time, preserve $parsedFunction->name to __ (the original name) and determinate easily whether this parsedFunction holds a domain (and as such map it to gettext or dgettext).

Unrelated but I found strange that public function setFunctions(array $functions): self must be inherited that way public function setFunctions(array $functions): parent.

[oscarotero]

Ok, I understand now.

Anyway, I think changing the function name is a bit hacky. A ParsedFunction represent a real function call in the code. How do you handle this function is a different thing, and other classes should be responsibles for that.

For example, for this case, I'd create a WordPressScanner class, maybe extending the CodeScanner and overriding the handleFunction method with other that choose the best handler for each case, according the number of arguments, not only the function name.

[drzraf]

https://github.com/drzraf/Twig-Scanner/blob/master/src/ParsedFunction2.php Working-around this limitation without touching the ParsedFunction was immensely painful and implied either large and ugly code either not inheriting CodeScanner at all.

[drzraf]

https://github.com/php-gettext/Gettext/pull/195

I think the scanner can be improved because you're mapping the twig functions to php functions, keeping the same arguments in the same order in order to reuse the same handlers. I think this scanner should be more independent, but it's a good start.

By more independent, are you suggesting not inheriting the CodeScanner at all?

[oscarotero]

Not exactly. CodeScanner was too focused in collecting gettext functions (gettext, ngettext ...) but these functions are not common to all codes, so I've made this change: https://github.com/php-gettext/Gettext/tree/feature/functions-handlers-trait/src/Scanner (in other branch for now) in order to move some code of this class to the new FunctionsHandlersTrait, so CodeScanner remains a more generical class that can fit with all types of code.

I've updated also js-scanner and php-scanner to adapt to this change.

Twig-Scanner can extend CodeScanner but overriding the getFunctionHandler method to return the right handler for each twig function. Doing so, the handlers do not depend exclusively on the function name.

[oscarotero]

I close this and we can continue the conversation in https://github.com/php-gettext/Twig-Scanner/pull/1