chore: Tokens - cleanup

[keradus]

depends on #8349

[coveralls]

coverage: 95.036%. remained the same
when pulling abe5d12 on keradus:8234b
into a730f2f on PHP-CS-Fixer:master.

[mvorisek]

I advise to revert this, how does this PR make things better?

[keradus]

Happy to hear the reason for revert. Otherwise, hard to discuss.

Why I raised those changes?

• no need for logic to calculate amount of empty tokens when purpose of function is to remove all empty tokens
• one error fewer
• one extra assertion fewer

[mvorisek]

Sure 👍

The first assertion is good to make sure the check on l398 did really make it's job. (as it uses cache, this asserts the cache was in consistent state)

The 2nd assertion should be extended to check if really the removed empty tokens match the cache state, if yes, the cache can be set to zero.

As I discovered several bugs of cache update in the past, I find these assertions very valueable as hunting the bugs was very time consuming.

[keradus]

please convert those cases to tests. We barely have any assertions in src/ and I prefer to keep those ensuring mechanisms in tests.

[mvorisek]

That is not possible as you told me you do not want private internals tested.

[Wirone]

I am not a fan of assert() because I consider this as unreliable. But removing those asserts without providing replacement decreases code quality IMHO. Just my 2 cents.

[keradus]

I disagreed to have them added in first place, I believe Kuba too (and we both complained about messing with internals). they failed to us to provide value in first place.

i decided to merge PR as-is and apply fixes separately, instead of having another few weeks discussion in PR that was having good bunch of optimizations

[mvorisek]

I am not a fan of assert() because I consider this as unreliable. But removing those asserts without providing replacement decreases code quality IMHO. Just my 2 cents.

100%!