Broken isFirstTokenOnLine when T_WHITESPACE

[marcioAlmada]

Bug occurs with streams using the following configuration:

$stream->setIgnoreWhitespace(false);

Now every T_WHITESPACE token in the stream returns false positive for:

$token->isFirstTokenOnLine(); // always true

As a side effect, methods that depend on token->previousToken are failing too:

$token->getIndentation(); // throws exception "None has no value"

[marcioAlmada]

PS: confirmation needed.

[schmittjoh]

I believe we are not using this flag a lot; so, yes could be.

On Sun, May 11, 2014 at 12:13 PM, Márcio Almada [email protected]:

PS: Waiting for confirmation.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/6#issuecomment-42766872
.

[marcioAlmada]

Unless I'm doing some stupid assumption, problem might be here and was caused by this or this (looks very suspect). Still not sure.

[schmittjoh]

Could you check what "getLine()" outputs for these tokens? It's probably
wrong.

On Sun, May 11, 2014 at 12:37 PM, Márcio Almada [email protected]:

Unless I'm doing some stupid assumption, problem might be herehttps://github.com/schmittjoh/php-manipulator/blob/master/src/JMS/PhpManipulator/TokenStream/AbstractToken.php#L452-L454and probably was caused by
thishttps://github.com/schmittjoh/php-manipulator/blob/master/src/JMS/PhpManipulator/TokenStream.php#L156-L163or
thishttps://github.com/schmittjoh/php-manipulator/blob/master/src/JMS/PhpManipulator/TokenStream.php#L169-L179.
Still not sure.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/6#issuecomment-42767320
.

[marcioAlmada]

I made more complex tests before trying to isolate the problem. AFAIK, the line number is always right even with multiline source involved. Added line number check to the failing test.

[marcioAlmada]

Hi,

I just noticed that bug occurs ONLY when stream->setIgnoreWhitespace(false) is called AFTER stream->setCode($source). IMMO this is bad because API does not enforces correct usage of the configuration methods in any way.

Some suggestions:

• Enhance / Fix the behavior of ::setIgnoreWhitespace so it works as a real flag that can be changed at any time, even while moving across the stream (not sure if this is an easy task with current code base).
• Add a new arguments to ::setCode and deprecate ::setIgnoreWhitespace so no one will use the API in the wrong way.

public function setCode($code, $ignore_whitespace=false, $ignore_comments=false)
{

Or maybe any other idea? I could send a pull request if you agree.

😄

[schmittjoh]

I'd suggest just adding an exception for now.

On Sun, May 11, 2014 at 11:51 PM, Márcio Almada [email protected]:

Hi,

I just noticed that bug occurs ONLY when
stream->setIgnoreWhitespace(false) is called AFTER
stream->setCode($source). IMMO this is really bad because API does not
enforces correct usage of the TokenStream configuration methods in any way.

Some suggestions:

• Enhance / Fix the behavior of ::setIgnoreWhitespace so it works as a
  real flag that can be changed at any time, even while moving across the
  stream (not sure if this is an easy task with current code base).
• Add a new arguments to ::setCode and deprecate ::setIgnoreWhitespaceso no one will use the API in the wrong way.

public function setCode($code, $ignore_whitespace=false, $ignore_comments=false){

Or maybe any other idea? I could send a pull request if you agree.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/6#issuecomment-42784448
.

[marcioAlmada]

Made a pull request. Still think that modifying ::setCode could be more intuitive than handling an exception though.

Thanks you for the support. Cheers!