Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX] Fix math expression error verbosity #396

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

NamelessCoder
Copy link
Member

Prevents seeing the “non-numeric value encountered”
error from PHP when a template attempts to do math
on a variable that is not declared, or does not have the
right type. See code comments for exact behavior.

The intention is to match PHP type coercion as close
as possible - among other things by inferring NULL as
value when a variable is most likely not defined.

Copy link
Member

@mbrodala mbrodala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK from reading but we should really add tests for this.

@NamelessCoder
Copy link
Member Author

Now with 100% code coverage.

@NamelessCoder
Copy link
Member Author

In case you didn't notice... this actually sneaks in neat ability:

<f:render partial="MyPartial" arguments="{_all + another}" />

Doesn't allow you to define the array inside the expression, only works on variables - but works.

@mbrodala
Copy link
Member

I did notice and I think this should be done as a separate change to give it the spotlight it deserves. ;-)

@NamelessCoder
Copy link
Member Author

Something to consider: should we allow left/right side to be null when the other is an array and the operator is +? In which case, we just return whichever side was not-null (implied coercion of (array) $valueThatIsNull).

@NamelessCoder
Copy link
Member Author

I hesitate to do this array union as a separate change exactly because it won't allow the array to be built directly in the expression. It's also not as easily done with another expression node type. It would be far easier to handle if that lexer I'm working on was a reality.

If/when we do that, I expect it will instead look a bit like arguments="{_all} + {foo: 'bar'}" - in other words, a change that affects how arrays are detected and built, not something that affects an object accessor.

@mbrodala
Copy link
Member

With separate change I mean separate PR with exactly the same code as here, not some kind of different syntax or so.

@NamelessCoder
Copy link
Member Author

Yes, I understood that, but I'd argue that this isn't a feature or a separate bugfix - it still falls into the category of this same error prevention (by guarding against arrays being divided, for example). You could still do this before, you'd just receive some nasty errors in pretty much all contexts if you didn't do it 100% correctly. Now you don't :)

Prevents seeing the “non-numeric value encountered”
error from PHP when a template attempts to do math
on a variable that is not declared, or does not have the
right type. See code comments for exact behavior.

The intention is to match PHP type coercion as close
as possible - among other things by inferring NULL as
value when a variable is most likely not defined.
Copy link

@foobar13372 foobar13372 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That it is finally fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants