Description Thierry BLIND 2019-09-30 05:31:29 EDT

Hi,
while working on the formatter JUnit tests for PHP 7.4, I've seen that 2 tests always failed with following errors:


java.lang.AssertionError
	at org.eclipse.php.formatter.core.CodeFormatterVisitor.visit(CodeFormatterVisitor.java:2772)
	...

The 2 files are org.eclipse.php.formatter.core.tests/workspace/formatter/php5/default_formatter_configuration/phpWithHtml1.pdtt and org.eclipse.php.formatter.core.tests/workspace/formatter/php5/new_default_formatter_configuration/phpWithHtml1.pdtt

Strangely this (long standing) bug was never problematic for our PDT gerrit builds, so it stayed unnoticed...
Also an "assert" will not be "seen" by the users, so it was never reported.

The reason that we hit "assert false;" in class CodeFormatterVisitor is pretty simple to understand: between 2 PHP code regions we must/should only have one inline-html region. We should never have multiple successive InLineHtml objects, 
since we don't analyze (and split) HTML content, except with class PHPTokenizer that isolates PHP code sections inside HTML files.

So why do we have multiple successive inline-html regions?
It comes from 2 rules inside the ast_scanner.flex files:

<YYINITIAL>(([^<]|"<"[^?%s<])+)|"<s"|"<" {
	...
}
<YYINITIAL>"<?"|"<script"{WHITESPACES}"language"{WHITESPACES}?"="{WHITESPACES}?("php"|"\"php\""|"\'php\'"){WHITESPACES}?">" {
	...
}

The lexers are trying to detect if inline-html regions contain "<script language='php'>" text. In this case it will considered to be a php region, otherwise we create a new T_INLINE_HTML symbol each time we find "<s" text !!!
This "bad" side-effect is pretty easy to reproduce since we have some very common html tags starting with "<s", like <span>, <select>, <section> or <strong> tags.

Here is an example taken from the phpWithHtml1.pdtt file that will show the problem (in debug mode):
<?php if (isset($this->previous)):  ?>
<?php else :    ?>
  <span class="disabled">&lt; previous</span> |
<?php endif;   ?>

This 2 rules seem clumsy. We only handle <?, <?=, <?php (and obsolete <%, <%=) in class PHPTokenizer as PHP region delimiters, but we additionaly handle "<script language='php'>" tags in the PHPAstLexer classes. We anyway don't look for closing "</script>" tags, so I'm not sure those script tags are correctly handled...

For sure, PHP code inside "<script language='php'>" tags is badly highlighted and recognized by PDT. And this syntax is deprecated since PHP 7.0 :

https://www.php.net/manual/en/language.basic-syntax.phptags.php

I don't want to write specific code everywhere in PDT to correctly handle those deprecated script tags (that are removed since PHP 7.0) but do a minimal fix:
- remove those "<script language='php'>" rules from the ast_scanner.flex files starting from PHP 7.0
- add a workaround in class CodeFormatterVisitor to gracefully handle multiple successive InLineHtml objects for PHP < 7.0 code

Maybe we should even totally remove "<script language='php'>" from all ast_scanner.flex files, since we never really supported it, it would avoid clumsy workarounds...

@Dawid, what do you think?

Thierry.