Skip to content

Conversation

@natsuki-engr
Copy link
Contributor

@natsuki-engr natsuki-engr commented Feb 4, 2025

  • faq
  • common techniques
  • cookbook

Copy link
Collaborator

@DerManoMann DerManoMann left a comment

Choose a reason for hiding this comment

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

Good stuff! Could you just please rename the files to something more meaningful? For example server_xx.php

@natsuki-engr
Copy link
Contributor Author

@DerManoMann
Thanks for checking my PR!
In the faq, the annotation sample is shown as an example of "dummy class".
So I chose the filename "dummy_class_xx.php" - was that not appropriate?

The simplest solution to avoid this issue is to add a 'dummy' class to the docblock

Please let me know if my understanding is incorrect. 🙏

@DerManoMann
Copy link
Collaborator

My bad - yes, this is fine.

@DerManoMann
Copy link
Collaborator

I think it would be good to add a script to validate that the snippets do evaluate to the same spec... I can look into that at some point.

@DerManoMann
Copy link
Collaborator

One think I noticed is that the attribute examples use double quotes for string. Code style here is to use single quotes unless there is actual need to use double. Might be a pain, biut probbly good to get in. I think IDE can help with that.

@natsuki-engr natsuki-engr marked this pull request as ready for review May 7, 2025 07:46
@natsuki-engr natsuki-engr requested a review from DerManoMann May 7, 2025 07:50
@natsuki-engr
Copy link
Contributor Author

natsuki-engr commented May 12, 2025

Hi, @DerManoMann
CI test tests/DocSnippetsTest::testSnippets fails on some snippets because of mismatch of operationId.
I guess the snippets or test should be fixed.

For example, an error on file_upload_with_headers_at.php:

1) OpenApi\Tests\DocSnippetsTest::testSnippets with data set "file_upload_with_headers-3.0.0" (array('/workspaces/swagger-php/tests...an.php', '/workspaces/swagger-php/tests...at.php'), '3.0.0')
Snippet: $/workspaces/swagger-php/tests/../docs/snippets/guide/cookbook/file_upload_with_headers_at.php > paths > /v1/media/upload > post > operationId
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'6e2b268bb931464252c917c84b82cba5'
+'02d11fc2bbdec221ec8917f8a596bddd'

/workspaces/swagger-php/tests/OpenApiTestCase.php:175
/workspaces/swagger-php/tests/OpenApiTestCase.php:165
/workspaces/swagger-php/tests/OpenApiTestCase.php:165
/workspaces/swagger-php/tests/OpenApiTestCase.php:165
/workspaces/swagger-php/tests/OpenApiTestCase.php:165
/workspaces/swagger-php/tests/DocSnippetsTest.php:57

The operationId mismatch is caused by a difference in the namecepace of the copies created by DocSnippetsTest::testSnippets. Possible solutions are:

  1. Modify assertSpecEquals to skip the operationId check
  2. Explicitly specify the operationId value within the snippet (as in minimal_api_*.php)

However, neigher apprach seems particulary good. Do you have any better ideas?

@DerManoMann
Copy link
Collaborator

@natsuki-engr I did push a fix to master that disables operationId generation - that should help. Sorry, no better idea 🤷

I guess you can still put in explicit ids where it makes sense, but for the purpose of this test they are not that relevant IMO.

Nice work, btw.

@natsuki-engr natsuki-engr force-pushed the doc-add-attribute-examples branch from 7185ac9 to e04d06b Compare May 13, 2025 01:15
Copy link
Collaborator

@DerManoMann DerManoMann left a comment

Choose a reason for hiding this comment

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

A few places where use OpenApi\Annotations as OA; is missing - would be great to get those added.

Another thing is that we now should add the docs/snippet folder to CS - for that you'd need to update .php-cs-fixer.dist.php to add the folder; something like this:

// ...
$finder = PhpCsFixer\Finder::create()
    ->path('src')->name('*.php')
    ->path('tools')->name('*.php')
    ->path('docs/examples')->name('*.php')
    ->path('docs/snippets')->name('*.php')
    ->path('tests')->name('*.php')

Other than that it looks great. Thanks for taking the time.

@natsuki-engr
Copy link
Contributor Author

@DerManoMann
Thank you so much for fixing the test code. It seems to be working fine.

I apologize for the missing many use statements. I've added them, please check it again.
I also added CS check for docs/snippets directory, and the resulting formatting changes are included in commit 66d7e95.
Thanks again for your help!

@DerManoMann
Copy link
Collaborator

No problem. Any help is appreciated and improving the docs is always valuable.

@DerManoMann DerManoMann merged commit 4d7c5f6 into zircote:master May 14, 2025
23 checks passed
@DerManoMann
Copy link
Collaborator

@natsuki-engr Deployed and looking real nice!

As a side note I noticed some examples have corresponding YAML - it wonder if adding a new tab for the generated spec would work ...

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.

5 participants