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

Semicolon denoted as inexpected in PHP code within JavaScript #836

Closed
gillesVanderstraeten opened this issue Dec 6, 2019 · 10 comments
Closed

Comments

@gillesVanderstraeten
Copy link

This code denotes the final semicolon as inexpected:

<script>
  let foo = <?= 'foo' ?>;
</script>

This behaviour since version 1.3.1.

@bmewburn
Copy link
Owner

bmewburn commented Dec 6, 2019

1.3.x now gives diagnostics for embedded js. This is done via multiple language servers. Intelephense understands php but not js and js (actually typescript) server understand js but not php which results in the reported error above.

This is difficult to resolve and I think there will always be many edge cases here because valid js is only produced after the script is run. Some solutions could be.

  1. Replace the php section with an empty string so the js server thinks it is correct. This would only solve this case but it may be the most common.
  2. Ignore the first error after a php range. Though, this would rely on the js server being able to recover on the first parse error and legitimate errors could be missed.
  3. Add an option to turn off embedded diagnostics altogether.

There's also a workaround which IMO is a cleaner way to supply data to js scripts. Add data to the DOM as a JSON object.

<script type="application/json" id="my-data">
    <?= json_encode($myData); ?>
</script>

<script>
var myData = JSON.parse(document.getElementById('my-data').innerHTML);
</script>

@gillesVanderstraeten
Copy link
Author

I resolve it by surrounding the PHP section with quotes:

<script>
  let aVariable = '<?= $myData ?>';
</script>

But an option to turn off embedded check could be a good thing.

@bmewburn
Copy link
Owner

Added intelephense.diagnostics.embeddedLanguages setting in 1.3.3 which will enable/disable all embedded language diagnostics. Will leave this open to investigate other solutions.

@coling
Copy link

coling commented Dec 11, 2019

Just because you can create a slightly different version of this same error depending how you include the semicolon, here's a further example.

image

@ghnp5
Copy link

ghnp5 commented Dec 18, 2019

@gillesVanderstraeten "I resolve it by surrounding the PHP section with quotes:"

The problem comes when you have booleans.
And you really shouldn't have to change your code just to accommodate these false positives. :)

@KapitanOczywisty
Copy link
Contributor

And you also shouldn't expect miracles from editor. Passing data directly into JS is bad idea, you really should use data-* attributes or <script type="application/json">.

You can go with this

<script type="text/javascript">
var data = function(name){
  try {
    return document.currentScript.dataset[name];
  } catch(e){}
};
</script>
<!-- ... -->
<script type="text/javascript" data-var1="true" data-var3='{"foo":"bar"}'>
console.log(data('var1'), data('var2'), data('var3'));
</script>

Or even this

<script type="text/javascript">
var data = new Proxy({},{get(target,name) {
  try {
    return document.currentScript.dataset[name];
  } catch(e){}
}});
</script>
<!-- ... -->
<script type="text/javascript" data-var1="true" data-var3='{"foo":"bar"}'>
console.log(data.var1, data.var2, data.var3);
</script>

Almost everything is better than PHP generating JS code.

@ghnp5
Copy link

ghnp5 commented Dec 18, 2019

@KapitanOczywisty
Not really. I do whatever I want with my code, as long as it's good syntax and valid programming. It's not like it's such bad code that it's completely unacceptable/unmaintanable.

It's perfectly valid to do what the people in this thread are doing.

The parser has a problem and that needs to be fixed.
For now, I disabled the setting and will continue doing it my way.

@KapitanOczywisty
Copy link
Contributor

KapitanOczywisty commented Dec 18, 2019

This code shows errors! fix your extension, because this is the way I like to write my code.. 😕

<script type="text/ja<?='vascript">'?>
eva<?php
print("l('a");
?>ler<?='t("Test")'?>');
</script>

For anybody who cares:

  • inline js is considered insecure - it's good idea to block it in CSP
  • it's easier to debug code if you don't mix languages, team and future you may waste hours to decrypt past creations
  • one json object instead of multiple echos means less potential points of failure and any BC break is way easier to fix in one place.

@woodpile72
Copy link

woodpile72 commented Dec 30, 2019

I agree with @ghnp5. I opened another bug ( #913, now closed) because I wasn't aware of this one. The fact of the matter is the behavior of your extension changed significantly and now all my docs are littered with red and error counts.

If this is a "won't fix" situation, then I move to another extension and I don't dontate to your cause. Fix it and I'll donate immediately. Bit of advice: use your money-making, marketing brain and not your geek, perfectionist brain - you can't make everyone change their code. :-)

@bmewburn
Copy link
Owner

bmewburn commented Jan 6, 2020

In 1.3.7 1 is substituted for php expressions in embedded languages. This hopefully takes care of the more common case of let foo = <?= 'foo' ?>; . I don't think this will ever have a comprehensive solution due to reasons already noted here. So more of a "can't fix" than a "wont fix".

Closing this ticket as it has been actioned by providing a way to disable embedded diagnostics and by the substitution above. Any other examples of false embedded language diagnostics should be opened in a new ticket with code examples.

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

No branches or pull requests

6 participants