Afform linter has only been running on core-core
Overview
There is a linting test for Civi's Angular-HTML files (tests/phpunit/Civi/Angular/PartialSyntaxTest.php
). However, the linting is not applied consistently, and several files are not in compliance.
Having files which pass the linter ensures that:
- Modified/forked files can be easily compared to their originals.
- Files can be filtered/processed via
hook_alterAngular
.
Current behaviour
PartialSyntaxTest
scans all available Angular modules at the time of test. In effect, it scans civicrm-core:ang/**.html
.
When the test runs, search_kit
and afform
are not active, so not civicrm-core:ext/search_kit/ang/**.html
and civicrm-core:ext/afform/*/ang/**.html
are not scanned.
NOTE: This mechanism is built on top of PHP's DOMDocument
(generally) and phpQuery
(specifically). To test an HTML partial, we basically say:
$doc = (new DOMDocument(...))->loadHTML($inputString);
$outputString = $doc->saveHTML();
assert $inputString === $outputString
So a document is considered good if it is stable/consistent across reads+writes.
Expected behaviour
All Angular HTML in civicrm-core.git
should be scanned by a linter.
All files should pass.
Comments
There is a helper script tools/scripts/check-angular.php
which can validate+reformat file. The script is not super-sophisticated -- but just to get a sense of it, one might call:
php tools/scripts/check-angular.php --colordiff $PWD/ext/search_kit/ang/crmSearchTasks/crmSearchTaskUpdate.html
In principal, we could run this on all the HTML files and simply commit the reformatted version. Then we might rework PartialSyntaxTest
to plug the hole in the workflow.
But...
There's the issue of the actual changes in the formatting. It is important to note that PHP's DOMDocument
has two formatting modes -- HTML and XML. The parser here has to use one or the other. These differ in several small ways -- eg some notations work the same in both, but some notations are rejected by one while accepted by the other. Going from memory (based on testing yesterday), here are some notable differences:
Topic | Example | HTML Mode Support | XML Mode Support | Comment |
---|---|---|---|---|
Standard tag, text | <div>Hello</div> |
|
|
div , span , p , etc |
Standard tag, empty-text | <div></div> |
|
(Prefer <div/> ) |
div , span , p , etc |
Standard tag, self-closing | <div/> |
(Prefer <div></div> ) |
|
div , span , p , etc |
Standard tag, self-closing, implicit | <input> |
|
|
input , img , hr , br , etc |
Standard tag, self-closing, explicit | <input/> |
(Prefer <input> ) |
|
|
Custom tag, text | <custom>my text</custom> |
|
|
|
Custom tag, empty-text | <custom></custom> |
|
(Prefer <custom/> ) |
|
Custom tag, self-closing | <custom/> |
(Prefer <custom></custom> ) |
|
|
Value-less attributes (custom or standard) | <foo ng-list> |
|
|
|
Value for disabled
|
<foo disabled="abcd"> |
|
|
(I wanted to see this table because I had the nagging feeling that we were boxed in somehow -- and this does show that there are errors in either case. But these can be worked through...)
I believe that running the auto-format (at this moment) would pose a few issues:
- For HTML mode (status-quo), it would discard the
disabled="..."
that appears in ~5 components. (I suspect that this status-quo is actually a bug -- but only symptomatic when usingalterAngular
.) We could update affected files. This may require changing some contracts. - For XML mode, it would fail to parse
<input>
,<foo ng-list>
, and similar. We could update these (<input/>
and<foo ng-list=",">
), though this would affect many more lines of code. - In either mode, some files need to flip between
<foo></foo>
and<foo/>
. Regex can control this a bit more tightly, though. We would need to choose which form we like.
There's also the possibility (out of left field) to replace the current parser or formatter or validator. But that feels like an even bigger rabbit hole.