Move tabs/tab directive to rst package#1316
Move tabs/tab directive to rst package#1316wouterj wants to merge 2 commits intophpDocumentor:mainfrom
Conversation
This directive is also very useful outside of the Bootstrap theme (e.g. it is used by the Symfony docs with their custom theme). This moves the implementation to the guides-restructured-text and provides a smooth upgrade path in the guides-theme-bootstrap package.
After initializing the Twig Environment, no new variables can be introduced (existing ones can be changed). As Environment::load() initializes the environment, this causes issues when calling renderTemplate() afterwards. By defining them in both methods, no error is thrown.
| "php": "^8.1", | ||
| "phpdocumentor/guides": "^1.0 || ^2.0", | ||
| "phpdocumentor/guides-restructured-text": "^1.0 || ^2.0" | ||
| "phpdocumentor/guides-restructured-text": "^1.10 || ^2.0" |
There was a problem hiding this comment.
this package should also depend on doctrine/deprecations now that it uses it directly, instead of relying on the transitive dependency coming from phpdocumentor/guides
| try { | ||
| $twig = $this->environmentBuilder->getTwigEnvironment(); | ||
| $twig->addGlobal('env', $context); | ||
| $twig->addGlobal('debugInformation', $context->getLoggerInformation()); |
There was a problem hiding this comment.
The proper fix is probably to change the EnvironmentBuilder to define those global variable first, so that TwigTemplateRenderer::renderTemplate() ends up updating the value of existing global variables (which is allowed) instead of maybe defining new global variables (which is not allowed anymore after starting to render templates)
| public function __construct( | ||
| protected readonly string $name, | ||
| protected readonly string $plainContent, | ||
| protected readonly InlineCompoundNode $content, |
There was a problem hiding this comment.
Should those really be promoted properties if they are passed to the parent ? Isn't this overriding the parent properties (which has much bigger BC risk than using them, as property overrides in PHP enforce invariant types and modifier compatibility) ?
2 participants
Symfony is using the
.. tabs::directive with our custom theme instead of BootstrapCSS. I can see this use-case for other users as well, so I propose to move the directive to the restructured-text sub-package. I provided a basic HTML template that does not work, but has enough to make a working JS implementation.I've used the technique used by Symfony for moving a class to another namespace. This provides the smoothest upgrade path, with a deprecation notice for the old class, but no break in runtime code or static analysis.
At last, it looks like the
GeneralDirectiveNodeRendererdid not function properly and always threw a Twig exception when defining the global variables. I've applied a basic fix to theTwigTemplateRendererfor this, to make the new tabs test pass.