phpDocumentor Guides integration#200
Conversation
| /* | ||
| * This file is part of the Docs Builder package. | ||
| * (c) Ryan Weaver <ryan@symfonycasts.com> | ||
| * This file is part of the Guides SymfonyExtension package. |
| { | ||
| $fqcn = u($content)->replace('\\\\', '\\'); | ||
|
|
||
| $url = \sprintf($this->buildConfig->getSymfonyRepositoryUrl(), $fqcn->replace('\\', '/').'.php'); |
There was a problem hiding this comment.
This is loosing the changes done in recent PRs on this repo related to the support of the AI and UX monorepos.
| @@ -0,0 +1,275 @@ | |||
| { | |||
| "aliases": [ | |||
There was a problem hiding this comment.
Why are those highlight files new files instead of keeping (or moving) the existing files ?
| }, | ||
| "require": { | ||
| "php": ">=8.3", | ||
| "php": ">=8.2", |
There was a problem hiding this comment.
Why adding support for older versions ?
There was a problem hiding this comment.
Note. On symfony.com we run 8.4 and soon, 8.5 so we can safely use modern PHP versions.
| 'symfony-version', | ||
| null, | ||
| InputOption::VALUE_REQUIRED, | ||
| 'The symfony version of the doc to parse.', |
There was a problem hiding this comment.
Is this global CLI option defined elsewhere or is it gone ? Removing it might break the symfony.com website integration.
| $this->filesystem->put('index.rst', $contents); | ||
| } | ||
|
|
||
| #[\Override] |
There was a problem hiding this comment.
we don't use this attribute in Symfony (and Psalm has a config setting to avoid requiring its usage)
| { | ||
| $this->application = new BaseApplication(); | ||
| $this->buildConfig = new BuildConfig(); | ||
| public function __construct( |
There was a problem hiding this comment.
This change breaks the bin/docs-builder script currently.
There was a problem hiding this comment.
@javiereguiluz is the symfony.com website using the docs-builder executable or is it using this package as a library ?
There was a problem hiding this comment.
We use it as a library
There was a problem hiding this comment.
Could you share the integration code ? It would make it easier to figure out how to avoid (or at least reduce) BC breaks.
There was a problem hiding this comment.
For full docs (Symfony Docs, bundles, the Symfony Book) it's basically this:
$buildConfig = (new BuildConfig())
->setContentDir($buildDir)
->setOutputDir($tempBuildDir)
->setImagesDir($publicImagesDir)
->setImagesPublicPrefix($publicImagesPrefix);
if ('symfony' === $bookSlug) {
$buildConfig->setSymfonyVersion($branch);
}
try {
$result = $this->docBuilder->build($buildConfig);
if (!$result->isSuccessful()) {
// ...
}
} catch (\Throwable $e) {
// ...
}For blog posts, it's just this:
$parseResult = (new DocBuilder())->buildString($rstContents);
if (!$parseResult->isSuccessful()) {
// ...
}There was a problem hiding this comment.
@wouterj this means it would be great to revert changes done to the DocBuilder public API if possible (I think we could expose the same public API than today based on top of guides) as it is the public API of this library.
Note that I don't even see where your new code uses the modified DocBuilder API (except in tests that haven't been updated and so are failing)
There was a problem hiding this comment.
Yes yes. I now also have access to the code behind the symfony.com's docs rendering. So I've started working on the integration (and making the new SymfonyExtension installable is a big part of that). Small steps, but we'll get there :)
|
Thanks for the detailed review, @stof :) This was just a blunt import of the commits I made in my test project, which I started from scratch. So the goal now is to make sure the code in this PR matches whatever we were doing in this repository before. Your comments are a great start with this, as they show some unintentionally difference that we should revert from this PR before merging. |
|
The CI also tells me that existing tests must be updated (which will help avoiding regressions if they can still run) |
3 participants
This merges the SymfonyExtension from https://github.com/wouterj/symfony-docs-guides into this project.