Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:98621
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain fleshgrinder.com from 77.244.243.83 cause and error)
Reply-To: internals@lists.php.net
References: <CAF+90c9vdsLuh1GAB4VPbYzJp=XiH6a5w79t5Kt=JLV6=xkdDA@mail.gmail.com>
 <16.06.40046.20A35D85@pb1.pair.com>
To: Andrea Faulds <ajf@ajf.me>, internals@lists.php.net, nikita.ppv@gmail.com
Message-ID: <153a6a0f-1c22-6560-8bca-584e92915840@fleshgrinder.com>
Date: Fri, 24 Mar 2017 19:35:16 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <16.06.40046.20A35D85@pb1.pair.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] Re: TOKEN_AS_OBJECT for token_get_all()
From: php@fleshgrinder.com (Fleshgrinder)

On 3/24/2017 4:23 PM, Andrea Faulds wrote:
> Hi Nikita,
> 
> Nikita Popov wrote:
> 
>> I'd like to add a new TOKEN_AS_OBJECT flag to token_get_all(), which
>> returns an array of PhpToken objects, rather than the mix of plain
>> strings
>> and arrays we currently have. The PhpToken class is defined as:
>>
>> class PhpToken {
>>     public $type;
>>     public $text;
>>     public $line;
>> }
> 
> Rather than adding a flag to token_get_all() to return objects, you
> could potentially instead make an equivalent static method on PhpToken
> (PhpToken::getAll() perhaps). That would avoid mixing “object-oriented”
> and “procedural” styles, though I don't know if it matters. It seems
> cleaner to me.
> 
> Thanks!
> 

I am also against adding another flag because it violates the single
responsibility principle. However, adding a `getAll` method to the
`PhpToken` data class also seems very wrong, because it violates the
single responsibility once again.

The real and proper solution is to have an actual PHP Lexer that is
capable of creating a token stream.

    class Lexer {
        public function __construct(string $source);
        public static function fromFile(string $path): self;
        public function tokenize(): TokenStream;
    }

    final class TokenStream implements IteratorAggregate {
        public function getIterator(): Generator<Token>
        public function toArray(): Token[]
    }

    final class Token {
        // Ideally this would be an enum, but...
        public const OPEN_TAG;
        public const CLOSE_TAG;
        // ...

        // I hope these are not mutable!
        public int $category;  // type
        public string $lexeme; // text
        public int $line;
        public int $column;    // we don't have that :(

        /** @see token_name */
        public function getCategoryName(): string;

        // We could add `is*` methods for the various categories here.
        public function isOpenTag(): bool;
        public function isCloseTag(): bool;
        // ...
    }

With this in place, we're ready for the future. The `TokenStream` can
easily use a generator over the internal array. Or we offer the
`toArray` method only at the beginning. Users will have to call it, but
that overhead is tiny, considering that we can extend upon it in the
future without introducing any kind of breaking change.

Obviously this could go into the namespace `PHP\Parser`, or we prefix
everything with `PHP` (I'd be for the former).

On a side note, going for `Php` instead of `PHP` is inconsistent with
the naming that is currently dominating the PHP core:

https://secure.php.net/manual/en/indexes.functions.php

There are already some things that violate it, e.g. `Phar` instead of
`PHAR`, but most things keep acronyms intact (`DOM`, `XML`, `PDO`, ...).
Introducing even more inconsistency to PHP seems like a very bad idea to
me. I know that this is considered bikeshedding by many people, but
consistency is very important and we are already lacking it on too many
levels as it is.

-- 
Richard "Fleshgrinder" Fussenegger