Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Documentation config exposed to the Generator #495

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ylsalame
Copy link

When in the process of creating a custom processor to filter Annotations based on custom properties, I realized there was no access to the configuration of the documentation itself. This code pushes the individual documentation config to the Generator so that the processor can use it.

…an benefit from it

When creating a custom processor, the documentation configuration is not Exposed to the Generator class. This makes any kind of documentation-based processing no possible.
…an benefit from it

When creating a custom processor, the documentation configuration is not exposed to the Generator class. This makes any kind of documentation-based processing not possible.
@DerManoMann
Copy link

It might be worth keeping in mind that swagger-php has also a system for passing configuration into processors.
This is available on the CL and via code and just relies on a naming convention and setters on the processor...

https://github.com/zircote/swagger-php/blob/master/tests/GeneratorTest.php#L127

/**
* @var array
*/
protected $config;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we be more precise and name this something like documentationConfig.

On the other hand - in the factory now we destruct whole documentationConfig and pass it's parts to generator as a separate params. This prop will hold everything now, so we will have duplication... 🤔

Comment on lines +114 to +115
?Filesystem $filesystem = null,
array $config
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null param should be last

@@ -229,7 +236,9 @@ protected function setProcessors(OpenApiGenerator $generator): void
$processors[] = $processor;
if ($processor instanceof \OpenApi\Processors\BuildPaths) {
foreach ($processorClasses as $customProcessor) {
$processors[] = new $customProcessor();
$customProcessorObj = new $customProcessor();
$customProcessorObj->config = $this->config;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

processors in swagger-php have no common interface, so this will only work if client will add config prop to it's processor. This will create a dynamic prop in the class now, but this is going to be deprecated in php8.2

So we need to thing about some internal interface, which has defined setConfig() method and only if custom processor implements it - do the setting.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have had similar thoughts, but since processors can also be callables this didn't get very far.
That's why I opened for checking for matching setXXX() setter methods...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants