Repository navigation
Conversation
…nsion Port the owncloud/config-to-docs converter to Node and embed it here, so the server config parameter pages no longer depend on a separate PHP repo (PHP/Twig/Laminas dependency problem, see owncloud/config-to-docs#32). - antora-extensions/lib/config-sample-to-adoc.js: dependency-free converter; reproduces the PHP tool's output byte for byte, quirks included, so the published pages do not change - antora-extensions/config-sample-to-adoc.js: maintainer-only extension, disabled in site.yml; pairs of source_path/target_path, report_only, stop_after_processing. Fails loudly on bad paths, unknown keys, a missing header marker or a missing $CONFIG array - config-docs-in-sync: run on Node 22 with the new converter instead of cloning config-to-docs and installing PHP 7.4 + Composer - content/server/AGENTS.md: the sample-comment authoring rules that previously lived in the config-to-docs README Output was verified against the PHP tool for the 10.16 and master core samples, and a quirk fixture generated by the PHP tool is the golden test. Signed-off-by: Lukas Hirt <info@hirt.cz>
oc-tmueller
left a comment
oc-tmueller
left a comment
There was a problem hiding this comment.
Reviewed the full diff against the PHP tool it replaces: its Twig template and the pinned phpdocumentor/reflection-docblock 4.3.4. No blocking issue. The new tests pass (9/9), and the current pages convert identically. The inline comments are latent divergences from the PHP output plus a few smaller points. I confirmed the first two by running the PHP regexes in php:7.4-cli next to the new code.
Two points on lines outside the diff:
.github/workflows/config-docs-in-sync.yml:108:$sample is missing in core $ref - the converter would report success and change nothingdescribed the PHP tool. The Node script throwsENOENTand exits non-zero on a missing input, so this message is now wrong. The pre-check is still useful for a clearer error. Please update the wording and the step comment to say so.extension-tests/static-files.test.js:245: the comment still lists the AGENTS.md/CLAUDE.md locations as(content/, content/ocis/, ui/). This PR addscontent/server/and updates the root AGENTS.md list, so the comment is now stale.
| [new RegExp(`^([^\\n]${WS}*)(?=-)`, 'gm'), ''], | ||
| [new RegExp(`[^\\n][^\\n] warning::${WS}*`, 'gm'), 'WARNING: '], |
There was a problem hiding this comment.
PHP ran these filters without /u, so [^\n] matched a single byte. In JS it matches a whole character (UTF-16 unit), so the two differ as soon as a line starts with a non-ASCII character. Checked against PHP 7.4:
é-mail here: PHP leaves it unchanged (éis two bytes, so the lookahead does not hit-), JS outputs-mail hereü warning:: x: PHP outputsWARNING: x(the[^\n][^\n]pair is the two bytes ofü), JS leaves it unchanged
No current sample triggers this, but the next one that does will silently publish something other than what the PHP tool would have. One way to match PHP is to run these filters over a latin1 view of the UTF-8 bytes (Buffer.from(s, "utf8").toString("latin1"), then convert back). Alternatively, add a test that pins the intended behaviour, if the divergence is acceptable.
| function splitDocBlock (comment) { | ||
| // A docblock that starts with a tag has no summary and no description. | ||
| if (comment.startsWith('@')) return { summary: '', description: '' } | ||
| const match = SPLIT.exec(comment.replace(/[ \t]+$/gm, '')) |
There was a problem hiding this comment.
reflection-docblock 4.3.4 strips trailing whitespace with \h*$ under /u, which also covers NBSP (U+00A0) and the other Unicode horizontal spaces. [ \t]+$ only covers space and tab.
The difference matters in renderDescription. A description line holding only an NBSP is emptied by PHP and skipped. In JS it survives, isn't phpTrim-empty (NBSP isn't in PHP's trim list), and counts as a line with zero indentation. That forces strip to 0, so the whole description keeps its indentation and may render as a literal block where PHP dedented it. /[\t \u00a0\u1680\u180e\u2000-\u200a\u202f\u205f\u3000]+$/gm would match \h.
| } | ||
| if (strip > 0 && strip !== Infinity) description = [lines[0], ...lines.slice(1).map((l) => l.slice(strip))].join('\n') | ||
| } | ||
| if (/\{@(?!\})/.test(description)) { |
There was a problem hiding this comment.
This throws on any {@ not followed by }. PHP's DescriptionFactory only treats well-formed {@tag ...} as an inline tag and renders it as text. Prose such as use {@ ... or a {@link https://...} passes through without error.
If core ever adds one of these, the required config-docs-in-sync check (nightly and on every docs PR) fails with a stack trace instead of a diff, and stays red until someone rewrites core's comment. Could this render inline tags the way PHP did, or at least match only well-formed {@tag and report the sample and line?
|
|
||
| // PHP's trim() default character list. | ||
| const PHP_WS = '[ \\t\\n\\r\\0\\x0B]' | ||
| const phpTrim = (s) => s.replace(new RegExp(`^${PHP_WS}+|${PHP_WS}+$`, 'g'), '') |
There was a problem hiding this comment.
Nit: phpTrim compiles a new RegExp on every call, and renderDescription builds another inside its per-line loop (line 96). With hundreds of blocks per sample, that adds up to a lot of identical compiles. Hoisting both to module-level constants, as SUMMARY_FILTER/CODE_FILTER already are, avoids that. The cost is small.
| console.error('usage: node scripts/config-sample-to-adoc.js <config.sample.php> <page.adoc>') | ||
| process.exit(2) | ||
| } | ||
| fs.writeFileSync(target, convert(fs.readFileSync(source, 'utf8'), fs.readFileSync(target, 'utf8'))) |
There was a problem hiding this comment.
This repeats the extension's read → convert → write path without its error wrapping. A missing target in CI gives a bare ENOENT stack, where the extension says cannot read target_path .... Any future change to that pipeline (an unchanged/updated report, for example) would also need making in two places. A shared regeneratePage(source, target) in lib/, used by both entry points, would keep them aligned.
| if (!Array.isArray(pairs) || !pairs.length) { | ||
| throw new Error('config-sample-to-adoc: `pairs` is required and must be a non-empty list') | ||
| } | ||
| const log = (message) => console.log(`[config-sample-to-adoc] ${message}`) |
There was a problem hiding this comment.
The other extensions here (llms-txt, sitemap-cleanup, prerelease-attribute, load-global-site-attributes) log through this.getLogger("config-sample-to-adoc"). Using console.log bypasses --log-level, --log-format json and --log-failure-level, so this output looks different from the rest of the build and can't be silenced.
What
Ports the
owncloud/config-to-docsconverter to Node and embeds it in this repo, so the server config parameter pages (config_sample_php_parameters.adoc,config_apps_sample_php_parameters.adoc) no longer depend on a separate PHP repo. This takes a different route than owncloud/config-to-docs#32 (Laminas/Twig upgrade): with the converter in the docs repo,config-to-docscan be retired and the Twig alerts go with it.Changes
antora-extensions/lib/config-sample-to-adoc.js: dependency-free converter. Output is byte-identical to the PHP tool, quirks included, so the published pages do not change. Carries the original MIT notice.antora-extensions/config-sample-to-adoc.js: maintainer-only extension,enabled: falseinsite.yml. Config:pairs(source_path,target_path),report_only,stop_after_processing. Runs onplaybookBuilt. Unlike the PHP tool it fails loudly on bad paths, unknown config keys, a missing// header endmarker and a missing$CONFIGarray.config-docs-in-syncnow runs on Node 22 against the new converter (viascripts/config-sample-to-adoc.js), with no PHP container,config-to-docscheckout or Composer install.content/server/AGENTS.md(+CLAUDE.mdsymlink, listed in the rootAGENTS.md) holds the sample-comment authoring rules that lived in theconfig-to-docsREADME.extension-tests/config-sample-to-adoc.test.js. The golden fixture was generated by the PHP tool, not by this code.Verification
report_onlyand write mode, and with a bad path.npm testpasses. Local runs were on Node 24; CI will be the first run on 22.Note: Antora camelCases extension config keys, so the YAML uses
source_pathetc. while the code readssourcePath. Unknown keys are rejected so a misspelledreport_onlycannot silently turn a dry run into a write.Follow-ups (not in this PR)
owncloud/coreto repoint the sample files' comments (currently "readme of theconfig-to-docsrepository") tocontent/server/AGENTS.md.owncloud/config-to-docsand close Discuss: replace abandoned zend-filter/zend-inputfilter with laminas/laminas-filter, laminas/laminas-inputfilter config-to-docs#32 as obsolete.config-docs-in-syncis a required check; this PR changes the job it runs, so it validates itself.