A token value might be a float

[PowerKiKi]

@derrabus, this a new version of #119, which might better explain the issue. I'll polish the code if we agree that this might get merged. What do you think ?

Closes #119

[greg0ire]

@PowerKiKi I addressed the CI issues. Now, is this really a bug? Or is it a new feature? Because right now, if you try to implement a lexer such as the one in your test, static analysis points out that you're doing something wrong.

[PowerKiKi]

Thanks you for your help with the CI.

I am coming from https://github.com/creof/doctrine2-spatial, that makes use of https://github.com/creof/wkt-parser, which in turns ended up using doctrine/lexer. My test case is basically a copy-paste of that method. So I am not a direct user of doctrine/lexer, but my project works with doctrine/lexer:^2.1, but crash with doctrine/lexer:^3.0, so from my point of view this is a bug.

But I suppose we could argue that it is a misusage by creof/wkt-parser ? Since I did not write any of those codebase, it's not my place to tell. I suppose it is up to you.

Also, in addition to all of that, the fork longitude-one/doctrine-spatial uses the fork longitude-one/wkt-parser. And while those forks are better maintained, they still use essentially the same code. So they are bound to see that failure too. I am not sure why it didn't happen yet :thinking: ...

I am thinking this comes down to the fact that PHP output arguments are not type checked. AbstractLexer::getType() requires to pass in a string, but it incorrectly assume that will come out as string|int, whereas it actually is mixed, because anything can be affected $value. So I think Token::__construct() must accept mixed, even though it is most often a string... but not always.

So what about we change that to the even more permissive mixed ?

[greg0ire]

I think we should stick with scalars, mixed feels like opening a can of worms. Let's retarget to 3.1.x and add |bool then?

[PowerKiKi]

Sure thing, I'll do that. Does the test looks reasonably written as-is ? or should it be refactored differently ?

[PowerKiKi]

The workflow needs to be approved for the CI to run...

[greg0ire]

It seems there are CI jobs failing. Please take a look at this guide for more on how to handle those.

[greg0ire]

Sure thing, I'll do that. Does the test looks reasonably written as-is ?

It looks good enough. Should the docs be modified as well to explain why one might want to use float or bool in a parser, if there is a good reason?

[PowerKiKi]

I pushed CI fixes, but I am afraid I won't be able to help for the documentation. I couldn't find an obvious place for it, and I don't think I am knowledgeable enough to write documentation for this lib.

[greg0ire]

Please address the remaining CI issues

[PowerKiKi]

Should be good to merge now

[PowerKiKi]

Does this PR needs more than one approval to be merged ? or can it already be merged ?

[greg0ire]

I'd like another review

[greg0ire]

Please kindly squash your commits together. If you don't, we'll try to remember to do it for you but it's best if you save us this trouble.

How to do that?

1. git rebase -i origin/3.1.x, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin/whatever-you-call-it repository.
2. A window will show up with many lines, replace pick with fixup on every line but the first one
3. Close your editor, git should do its magic, and you should end up with one commit
4. Use git push --force to overwrite what you already push. Don't forget the --force option otherwise git will try to merge both things together.

[PowerKiKi]

Squashed

[greg0ire]

Thanks @PowerKiKi !