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

Added PHPStan to verify code samples #2370

Merged
merged 57 commits into from
Jul 17, 2024
Merged

Added PHPStan to verify code samples #2370

merged 57 commits into from
Jul 17, 2024

Conversation

mnocon
Copy link
Contributor

@mnocon mnocon commented Apr 24, 2024

Target: master, 4.6. We might backport it to 3.3 if it's really needed, but it's EOM already and I'm not sure about the value

This PR adds PHPstan to verify code samples. I went through all errors and fixed those that looked doable in reasonable time.

The code is broken on purpose in 1aeab11

PHPStan requires two things from us:

  1. All code samples must be stored in code_samples and included from the doc - we cannot use ```php blocks, because they won't be validated
  2. Examples should be complete (they cannot reference classes that do not exist or are left as an exercise to the reader. It's possible to use stubs, as Paweł mentions below, but I believe they should be avoided if possible (to provide better examples).

When broken:
Zrzut ekranu 2024-07-2 o 18 44 32

the CI looks like this:
Zrzut ekranu 2024-07-2 o 18 44 55

Contains TMP commit, do not merge before it's removed

I have reviewed every code sample that has changed whether it has been included in the doc and adjusted the highlight and line numbers.

@Steveb-p
Copy link
Contributor

2. Examples must be complete (they cannot reference classes that do not exist or are left as an exercise to the reader)

This can be circumvented by using stubs, which are basically small PHP code declarations. They also overwrite any existing declarations (that is their primary use), which is useful if you cannot modify 3rd party code, but still want to have PHPStan compliant annotations.

Copy link
Contributor

Choose a reason for hiding this comment

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

As a general rule of thumb, try to separate each step in jobs with one empty line. This makes it easier for version control to pick up which sections of the jobs changed.

This is true for all YAML files. We do the same for service declarations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, thanks for the suggestion!

- name: Install php-cs-fixer
run: |
mkdir -p tools/php-cs-fixer
composer require --working-dir=tools/php-cs-fixer friendsofphp/php-cs-fixer --dev
Copy link
Contributor

Choose a reason for hiding this comment

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

You may want to create a composer.json file in that directory instead. This would make it easier to maintain code style locally, without this job. Additionally, you could use the Composer install action that we often use, and make use of it's caching.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this idea a lot - added in 17a93db

.github/workflows/build.yaml Outdated Show resolved Hide resolved
"friendsofphp/php-cs-fixer": "^3.9",
"phpstan/phpstan": "^1.10",
"phpstan/phpstan-symfony": "^1.3",
"ibexa/doctrine-schema": "5.0.x-dev",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This list is pretty long, but it's because the 5.0 is an unstable release - it will be shorter for 4.6

@mnocon mnocon changed the title [POC] Added PHPStan to verify code samples Added PHPStan to verify code samples Jul 2, 2024
@mnocon mnocon requested review from Steveb-p and a team July 3, 2024 05:56
Copy link
Contributor

@adriendupuis adriendupuis left a comment

Choose a reason for hiding this comment

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

Everything seems in place

@mnocon mnocon merged commit be149fd into master Jul 17, 2024
5 checks passed
@mnocon mnocon deleted the add-phpstan branch July 17, 2024 07:49
mnocon added a commit that referenced this pull request Jul 17, 2024
* Applied CI review suggestion

* Added basic PHPStan config

* Added PHPStan for ActivityLog examples

* Added CI

* [TMP] Added invalid code on purpose

* Fixed auth issues

* Fixed CS job

* PHP CS Fixes

* Rerun

* Simplified namespaces

* [TMP] Use the old namespace

* Generated baseline

* Reduced baseline

* Finished reviewing baseline

* PHP CS Fixes

* Rerun

* Removed autoloading

* [TMP] Break

* Adjusted lines

* Apply suggestions from code review

Co-authored-by: Konrad Oboza <konrad.oboza@ibexa.co>

* Added space separation

* Line offset fixes (#2430)

* checkout_api.md: Fix included lines after PHPStan fixes

* price_api.md: Fix included lines after PHPStan fixes

The confirmation output at the end of "Resolve prices" example was missing.

* create_custom_attribute_type.md: Fix highlighted line

Two lines removed in 086b885 but the highlighted line wasn't updated.

* search_api.md: Use `ContentTypeTermAggregation` lines

* oauth_client.md: Fix PROVIDER_PREFIX highlight

* segment_api.md: Fix highlighted line

One line added in aeee20a but the highlighted line wasn't updated.

(renaming from docs/api/public_php_api_managing_users.md to docs/users/segment_api.md in a9031bd)

* segment_api.md: Fix highlighted lines

One line added in aeee20a but the highlighted line wasn't updated.

(renaming from docs/api/public_php_api_managing_users.md to docs/users/segment_api.md in a9031bd)

* segment_api.md: Fix loadSegmentByIdentifier example intro

* search_api.md: Fix #sorting-results offset

* cart_api.md: Add missing EOF new line

* Added basic PHPStan config

* Added PHPStan for ActivityLog examples

* Added CI

* [TMP] Added invalid code on purpose

* Fixed auth issues

* Fixed CS job

* PHP CS Fixes

* Rerun

* Simplified namespaces

* [TMP] Use the old namespace

* Generated baseline

* Reduced baseline

* Finished reviewing baseline

* PHP CS Fixes

* Rerun

* Removed autoloading

* [TMP] Break

* Adjusted lines

* Applied CI review suggestion

* Apply suggestions from code review

Co-authored-by: Konrad Oboza <konrad.oboza@ibexa.co>

* Added space separation

* Revert "[TMP] Break"

This reverts commit ac2cfbb.

* Bumped CS-Fixer version

* Fixed CS

* Removed redundant doc block from WebinarEventTitleFulltextFieldMapper

* Added readme mention

* Added empty line to cart_api

* Removed CS-fixer dependencies

* Improved cart_api

* Update code_samples/recent_activity/src/Command/MonitorRecentContentCreationCommand.php

Co-authored-by: Adrien Dupuis <61695653+adriendupuis@users.noreply.github.com>

* PHP CS Fixes

* Removed TMP changes

---------

Co-authored-by: Konrad Oboza <konrad.oboza@ibexa.co>
Co-authored-by: Adrien Dupuis <61695653+adriendupuis@users.noreply.github.com>
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.

4 participants