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

MC-19366: Adds GraphQL sniffs #141

Merged
merged 25 commits into from
Sep 25, 2019
Merged

Conversation

jean-bernard-valentaten
Copy link
Contributor

Sniffing GraphQL schemas was added. The following rules/sniffs were added:

  • enum, type and interface names must be UpperCamelCase
  • Field names of type and interface must be snake_case
  • Argument names of fields must be camelCase
  • query and mutation names must be camelCase

In order to be able to tokenize a GraphQL file, we now make use of the lexer bundled in the package webonyx/graphql-php.

larsroettig
larsroettig previously approved these changes Sep 11, 2019
@lenaorobei
Copy link
Contributor

@jean-bernard-valentaten thanks, that is significant improvement!

Could you please demo it on public architecture meeting magento/architecture#273?

I see that some rules are still under discussion magento/architecture#269, so please be aware that we cannot merge it until final decision is made.

@lenaorobei lenaorobei added the on hold The issue or PR is on hold. label Sep 11, 2019
Copy link
Contributor Author

@jean-bernard-valentaten jean-bernard-valentaten left a comment

Choose a reason for hiding this comment

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

@lenaorobei Please discuss presentation of feature in architecture meeting with Igor Melnykov as he informed me, that it needs not be presented.

@lenaorobei
Copy link
Contributor

I pulled changes and run composer install. Running command php vendor/bin/phpcs --standard=Magento2 <path-to-my-magento>/app/code/Magento/CatalogGraphQl/etc/schema.graphqls gives me following error:
PHP Fatal error: Uncaught Error: Class 'PHP_CodeSniffer\Tokenizers\GRAPHQL' not found in magento-coding-standard/vendor/squizlabs/php_codesniffer/src/Files/File.php:556.

@jean-bernard-valentaten
Copy link
Contributor Author

@lenaorobei Thank you for the note, I fixed that issue and a couple of follow up issues

@lenaorobei
Copy link
Contributor

I run against Magento 2 app/code and following false-positive findings were discovered:


FILE: app/code/Magento/CmsGraphQl/etc/schema.graphqls
----------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------------------------
 19 | WARNING | Argument name "String@doc(description" is not in CamelCase format
----------------------------------------------------------------------------------------------


FILE: app/code/Magento/CustomerGraphQl/etc/schema.graphqls
-------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 246 WARNINGS AFFECTING 246 LINES
-------------------------------------------------------------------------------------------------------------------------------
   7 | WARNING | Argument name "@doc(description" is not in CamelCase format
 138 | WARNING | Enum value "AF@doc(description:Afghanistan)" is not in SCREAMING_SNAKE_CASE format
 139 | WARNING | Enum value "AX@doc(description:Åland Islands)" is not in SCREAMING_SNAKE_CASE format
 140 | WARNING | Enum value "AL@doc(description:Albania)" is not in SCREAMING_SNAKE_CASE format
...
-------------------------------------------------------------------------------------------------------------------------------


FILE: app/code/Magento/DownloadableGraphQl/etc/schema.graphqls
----------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------------------------------------------------------------------------------
 41 | WARNING | Enum value "FILE@deprecated(reason:`sample_url` serves to get the downloadable sample)" is not in SCREAMING_SNAKE_CASE format
 42 | WARNING | Enum value "URL@deprecated(reason:`sample_url` serves to get the downloadable sample)" is not in SCREAMING_SNAKE_CASE format
----------------------------------------------------------------------------------------------------------------------------------------------

Please add these cases to the unit tests as well.
Thanks.

Magento2/ruleset.xml Outdated Show resolved Hide resolved
Magento2/ruleset.xml Outdated Show resolved Hide resolved
@jean-bernard-valentaten
Copy link
Contributor Author

jean-bernard-valentaten commented Sep 17, 2019

I run against Magento 2 app/code and following false-positive findings were discovered:


FILE: app/code/Magento/CmsGraphQl/etc/schema.graphqls
----------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------------------------
 19 | WARNING | Argument name "String@doc(description" is not in CamelCase format
----------------------------------------------------------------------------------------------


FILE: app/code/Magento/CustomerGraphQl/etc/schema.graphqls
-------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 246 WARNINGS AFFECTING 246 LINES
-------------------------------------------------------------------------------------------------------------------------------
   7 | WARNING | Argument name "@doc(description" is not in CamelCase format
 138 | WARNING | Enum value "AF@doc(description:Afghanistan)" is not in SCREAMING_SNAKE_CASE format
 139 | WARNING | Enum value "AX@doc(description:Åland Islands)" is not in SCREAMING_SNAKE_CASE format
 140 | WARNING | Enum value "AL@doc(description:Albania)" is not in SCREAMING_SNAKE_CASE format
...
-------------------------------------------------------------------------------------------------------------------------------


FILE: app/code/Magento/DownloadableGraphQl/etc/schema.graphqls
----------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------------------------------------------------------------------------------
 41 | WARNING | Enum value "FILE@deprecated(reason:`sample_url` serves to get the downloadable sample)" is not in SCREAMING_SNAKE_CASE format
 42 | WARNING | Enum value "URL@deprecated(reason:`sample_url` serves to get the downloadable sample)" is not in SCREAMING_SNAKE_CASE format
----------------------------------------------------------------------------------------------------------------------------------------------

Please add these cases to the unit tests as well.
Thanks.

Fixed in af9c041 and c4069e0

I ran it (GraphQL analysis only!) against Magento 2 CE app/code and here is the output:

FILE: /home/valentatenj/workspace/adobe/magento2ce/app/code/Magento/VaultGraphQl/etc/schema.graphqls
----------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
----------------------------------------------------------------------------------------------------
  5 | WARNING | Argument name "public_hash" is not in CamelCase format
 29 | WARNING | Enum value "card" is not in SCREAMING_SNAKE_CASE format
 30 | WARNING | Enum value "account" is not in SCREAMING_SNAKE_CASE format
----------------------------------------------------------------------------------------------------


FILE: /home/valentatenj/workspace/adobe/magento2ce/app/code/Magento/QuoteGraphQl/etc/schema.graphqls
----------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------
 5 | WARNING | Argument name "cart_id" is not in CamelCase format
----------------------------------------------------------------------------------------------------

@lenaorobei
Copy link
Contributor

Please also address that following message appears on composer install step: Warning: The lock file is not up to date with the latest changes in composer.json. You may be getting outdated dependencies. Run update to update them.

@jean-bernard-valentaten
Copy link
Contributor Author

Please also address that following message appears on composer install step: Warning: The lock file is not up to date with the latest changes in composer.json. You may be getting outdated dependencies. Run update to update them.

Fixed in 4d99ee4

@lenaorobei lenaorobei merged commit 7813049 into develop Sep 25, 2019
@lenaorobei lenaorobei deleted the MC-19366-GraphQL-Code-Style-Test branch November 4, 2019 18:09
magento-devops-reposync-svc pushed a commit that referenced this pull request Dec 22, 2021
…kiy-magento-coding-standard-346

[Imported] Fix Issue 340
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants