Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

PHP syntax not recognized inside HTML quotes #409

Open
wasinwill opened this issue Jan 4, 2021 · 12 comments
Open

PHP syntax not recognized inside HTML quotes #409

wasinwill opened this issue Jan 4, 2021 · 12 comments

Comments

@wasinwill
Copy link

If an HTML code similar to style="background-image:url(<?= $image_url ?>)" is added to a .php file, Atom fails to recognize PHP tags if any character is entered after url(. See picture below:

Screenshot 2021-01-03 213839

After inspecting the elements with Developer Tools, it looks like it isn't theme-related. The example below shows when it works, with a code like url(<?= $image_url ?>). The tag is recognized and encapsulted inside a syntax--php element.

Screenshot 2021-01-03 214257

However, when a character is added after the bracket, such as url(/img/<?= $image_url ?>.jpg), the tag is no longer recognized and sits inside a syntax-css element.

Screenshot 2021-01-03 214359


Version 1.53.0 x64
Electron 6.1.12
Chrome 76.0.3809.146
Node v12.4.0
Theme One Dark

@KapitanOczywisty
Copy link
Contributor

I'm afraid this is won't fix issue. Syntax injections are broken by definition, since you can insert any piece of code in any place. In this situation there is no good way to fix php injection in another syntax block.

As I always say in these issues: try to avoid injections all together, there are better ways to pass data between languages.

@wasinwill
Copy link
Author

VS Code handles it perfectly though (see screenshot below).

Plus, PHP and HTML are meant to co-exist by design, and I think that syntax is a lot better than something like
<?php echo "<div class='img' style='background-image:url(/img/$image_id.jpg'></div>"; ?> as you keep HTML in HTML and PHP in PHP, especially for something this basic.

But however you'd prefer to write this, Atom isn't behaving as expected, and VS Code is, so I'm not sure why fixing this would be off-limits, while the people working on VS Code have clearly figured it out.

Screenshot 2021-01-04 074422

@Ingramz
Copy link
Contributor

Ingramz commented Jan 4, 2021

I wouldn't call it a wontfix before at least exploring why the injection doesn't work between text.

VSC doesn't parse CSS inside style attribute. This example is more suitable:

<style>
div {
  background-image: url(a<?= $image_url; ?>);
}
</style>

which based on my testing exhibits the same behavior where it isn't working mid-text.

@KapitanOczywisty
Copy link
Contributor

image

@wasinwill You were saying? VSCode and atom are using different syntax for css and html.

I'm not sure what is go-to nowadays for php/html templating, but I'm sure there are better ways which have full IDE support. Even going with attribute and propagating this with JS might be better.

@Ingramz We have so many problems like that with SQL and HTML that I'm rather confident this will be at least time consuming to mitigate. And I've checked what tokens are involved in this.

@wasinwill
Copy link
Author

@KapitanOczywisty I'm confused. Your screenshot shows that it doesn't work inside <style> tags (same for me) but does work in HTML on your end. So if we put aside <style> tags, you're saying you're not seeing the issue on your machine?

PS: I won't add JS to a file in order to fetch the URL of an image and set it as a CSS rule to a single div on a page just because my text editor won't highlight <?= $image_url ?>. Maybe I misunderstood what you meant. Now, if you guys are simply not willing to address this because you have bigger issues, that's understandable. I just figured I'd report it to contribute.

@KapitanOczywisty
Copy link
Contributor

KapitanOczywisty commented Jan 4, 2021

@wasinwill In style attr you don't have syntax highlighting in vscode, that is why it's working.

I'm saying in general, there is no point for one instance, but If you have more data passed from php to html like that It's good to think about template system. I've been writing code like that nearly 10 years and now when I need to do something in these projects I'm contemplating about my life choices. It's getting out of hands very quickly. Now I'm mainly working with React, so I'm not sure what would be good in your case.

To fix this we need to add special injection for that exact token. I'm not sure what would be long-term consequences performance-wise.

Edit: However, I'll think about that since you cannot use variable in url() in css. In other cases you can have e.g.

.count:after {
  content: 'The count is ' attr(data-count) '.';
}

@wasinwill
Copy link
Author

@KapitanOczywisty There are cases like setting background-image on the fly makes more sense for things that don't repeat, when the rest of the page is properly structured and easily maintainable, while the rest of the CSS is in a separate .css file.

Now, maybe you're right and fixing this will have consequences on performance and it's just not worth it. I guess it's a decision that you guys have to make.

@KapitanOczywisty
Copy link
Contributor

@wasinwill When testing I've found that it's working with single-quotes, so fix may be not needed after all.

<div style="background-image:url('/img/<?=$image_url?>.jpg')"></div>

image

@wasinwill
Copy link
Author

That works but prevents the use of double quotes inside the PHP tag (see below). But it works if used with caution.

Screenshot

@Ingramz
Copy link
Contributor

Ingramz commented Jan 5, 2021

I had some time to look into it and mostly it's about how CSS grammar consumes the contents of the url(). The injections in PHP never have a chance to consume the token as the following regular expression in CSS grammar consumes all of the characters at once:
https://github.com/atom/language-css/blob/7a9d4dbf8f2c0718a500758fa9a129f030ad6c65/grammars/css.cson#L2116

It is sufficient to just remove the + here for the PHP injections to work again at the cost that the parser needs to evaluate a character at a time, which should be no big deal.

@KapitanOczywisty
Copy link
Contributor

@Ingramz but then every single character is separate token, counting to 100 may be fast.

@Ingramz
Copy link
Contributor

Ingramz commented Jan 5, 2021

@KapitanOczywisty definitely a valid concern, but also fixable by re-arranging the grammar a bit. It looks like the pattern can be omitted entirely and replaced with a contentName property instead of the same value at the parent. Of course the other subpatterns will also have that scope then, but I'm not sure if this is a bad thing considering its name.

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

No branches or pull requests

3 participants