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.

Sign up for GitHub

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

Jump to bottom

#50 PHP-Scoper がテスト可能に #51

Merged
merged 14 commits into from
Jul 20, 2023
Merged

#50 PHP-Scoper がテスト可能に #51

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
04ae20f
#50 PHP-Scoper がテスト可能に
fumikito Jul 19, 2023
f6f4291
#50 再度チェックアウトする
fumikito Jul 19, 2023
34f8e37
#50 名前を修正
fumikito Jul 19, 2023
6b2758c
#50 再利用ワークフローからcomposite actionに
fumikito Jul 19, 2023
87e8427
#50 checkoutアクションをバージョン指定
fumikito Jul 19, 2023
3cd35dc
#50 action.ymlの指定は不要
fumikito Jul 19, 2023
a6e9ee5
#50 if 構文を変更
fumikito Jul 19, 2023
ac3cafa
#50 @openai: ignore
fumikito Jul 19, 2023
c7357f1
#50 Drop PHP 7.2 from test
fumikito Jul 19, 2023
cf05a2a
#50 Fix PHPUnit path
fumikito Jul 20, 2023
fa6c5ca
#50 Change if condition.
fumikito Jul 20, 2023
7f8c8c3
#50 構文エラーを修正
fumikito Jul 20, 2023
0df79ca
#50 Change if syntax again.
fumikito Jul 20, 2023
e9e8959
#50 fix syntax error in variable.
fumikito Jul 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions .github/actions/php-scoper/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
name: Setup PHP Scoper
description: Setup PHP Scoper
inputs:
Comment on lines +1 to +3
Copy link

Choose a reason for hiding this comment

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

🤖 OpenAI

The name and description fields in the GitHub Actions workflow file should be more descriptive. Consider updating them to provide clearer information about the purpose of the workflow.

scoper:
description: Whether to do PHP Scoper
required: true
Comment on lines +1 to +6
Copy link

@github-actions github-actions bot Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

🤖 OpenAI

The scoper input is marked as required, but it seems that the condition for executing the PHP Scoper steps is based on the value 'yes' instead of 'true'. This could lead to confusion and potential issues. It would be better to align the condition with the input description and make it consistent. Consider changing line 6 to:

required: true
default: 'no'

And changing lines 12 and 21 to:

if: ${{ inputs.scoper == 'yes' }}

This ensures that the condition matches the input value and avoids any ambiguity.


Comment on lines +1 to +7
Copy link

Choose a reason for hiding this comment

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

🤖 OpenAI

The scoper input parameter should have a default value of false instead of being required. This allows the workflow to run without explicitly specifying the scoper input. Additionally, the description for the scoper input should be updated to indicate that it controls whether PHP Scoper is enabled or not.

3: inputs:
4:   scoper:
5:     description: Whether to enable PHP Scoper
6:     required: false
7:     default: false

runs:
using: "composite"
Comment on lines +8 to +9
Copy link

Choose a reason for hiding this comment

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

🤖 OpenAI

The runs section of the workflow is using the composite strategy, which allows running multiple steps. However, there is only one step defined in this workflow. If there are no plans to add additional steps in the future, you can remove the using: "composite" line.

Comment on lines +1 to +9
Copy link

Choose a reason for hiding this comment

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

🤖 OpenAI

The new hunk introduces a GitHub Actions workflow file for setting up PHP Scoper. It includes inputs for the scoper parameter and defines multiple steps to execute based on the value of inputs.scoper. The workflow runs as a composite action.

steps:
- name: Add Prefix
if: 'yes' == inputs.scoper
shell: bash
run: |
composer global require humbug/php-scoper
~/.composer/vendor/bin/php-scoper add-prefix --output-dir=vendor-prefixed --force
composer dump-autoload --working-dir vendor-prefixed --classmap-authoritative
composer scoper-patch

Copy link

@github-actions github-actions bot Jul 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

🤖 OpenAI

The current implementation assumes that the PHP-Scoper tool is already installed globally. It would be better to include a step to install the tool if it's not already available. Additionally, the --output-dir option in the php-scoper add-prefix command should be set to vendor/vendor-prefixed instead of just vendor-prefixed. This ensures that the prefixed vendor files are placed in the correct directory.

11:     - name: Install PHP Scoper
12:       if: 'yes' == inputs.scoper
13:       run: composer global require humbug/php-scoper
14:
15:     - name: Add Prefix
16:       if: 'yes' == inputs.scoper
17:       shell: bash
18:       run: |
19:         ~/.composer/vendor/bin/php-scoper add-prefix --output-dir=vendor/vendor-prefixed --force
20:         composer dump-autoload --working-dir vendor/vendor-prefixed --classmap-authoritative
21:         composer scoper-patch

- name: Cleanup Vendor
if: 'yes' == inputs.scoper
run: rm -rf vendor
shell: bash

Copy link

Choose a reason for hiding this comment

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

🤖 OpenAI

The cleanup step (Cleanup Vendor) should only be executed if PHP Scoper is enabled (inputs.scoper == 'yes'). Otherwise, it could unintentionally delete the entire vendor directory, leading to issues during subsequent steps. Consider modifying lines 20-23 as follows:

- name: Cleanup Vendor
  if: ${{ inputs.scoper == 'yes' }}
  run: rm -rf vendor
  shell: bash

This change ensures that the cleanup step is conditional and only executed when necessary.

Copy link

Choose a reason for hiding this comment

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

🤖 OpenAI

The Cleanup Vendor step should be moved outside the if condition so that it always runs, regardless of whether PHP Scoper is enabled or not. This ensures that the vendor directory is cleaned up properly.

20:     - name: Cleanup Vendor
21:       run: rm -rf vendor
22:       shell: bash
23:
24:     - name: Skip PHP Scoper

- name: Skip PHP Scoper
if: ${{ inputs.scoper }} != 'no'
run: echo "Skipping Scoper"
shell: bash
Copy link

Choose a reason for hiding this comment

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

🤖 OpenAI

The Skip PHP Scoper step is unnecessary and can be removed. Since it comes after the previous steps, it will always be skipped if PHP Scoper is not enabled.

66 changes: 48 additions & 18 deletions .github/workflows/wordpress.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,13 @@ on:

jobs:
test:
runs-on: ${{ matrix.operating-system }}
runs-on: ubuntu-latest
strategy:
matrix:
operating-system: [ ubuntu-latest ] # OS. ubuntu-18.04 is also available.
php: [ '7.2', '7.4', '8.0' ] # PHP versions to check.
php: [ '7.4', '8.0' ] # PHP versions to check.
wp: [ 'latest', '5.9' ] # WordPress version to check.
tools: [ composer ]

scoper: [ 'yes', 'no' ]
services:
mysql:
image: mysql:8.0
Expand All @@ -29,9 +28,9 @@ jobs:
env:
MYSQL_ROOT_PASSWORD: root
MYSQL_AUTHENTICATION_PLUGIN: mysql_native_password
name: UnitTest on WordPress ${{ matrix.wp }} in PHP ${{ matrix.php }}
name: UnitTest WP ${{ matrix.wp }} in PHP ${{ matrix.php }} with PHP Scoper=${{ $matrix.scoper }}
steps:
- uses: actions/checkout@master
- uses: actions/checkout@v3

- name: Setup PHP with composer v2
uses: shivammathur/setup-php@v2
Expand All @@ -47,26 +46,59 @@ jobs:
- name: Install dependencies
run: composer install --prefer-dist --no-progress --no-suggest
Comment on lines 46 to 47
Copy link

Choose a reason for hiding this comment

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

🤖 OpenAI

Add the --no-dev flag to exclude development dependencies if they are not required for testing purposes.

50:
Make sure the ./github/actions/php-scoper/action.yml file exists and contains the necessary configuration.

53:
Ensure that the correct value is being passed to the scoper input.

57:
Review the files being made executable to ensure that only the necessary files are included and there are no security risks associated with this step.


Comment on lines 47 to 48
Copy link

Choose a reason for hiding this comment

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

🤖 OpenAI

The step installs dependencies using Composer. No issues found.

- uses: ./.github/actions/php-scoper
name: Do PHP Scoper if possible
with:
scoper: ${{ matrix.scoper }}

- name: Make Binaries Executionable
if: 'yes' == matrix.scoper
run: chmod +x vendor-prefixed/vendor/bin/*

- name: Start MySQL
run: |
sudo systemctl start mysql
mysql -h 127.0.0.1 --port 3306 -u root --password=root -e "ALTER USER 'root'@'localhost' IDENTIFIED WITH mysql_native_password BY 'root';"
Comment on lines 59 to 61
Copy link

Choose a reason for hiding this comment

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

🤖 OpenAI

A step is added to start MySQL and alter the root user's password. No issues found.

Comment on lines 60 to 61
Copy link

Choose a reason for hiding this comment

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

🤖 OpenAI

Ensure that the workflow environment has the necessary permissions to execute the sudo systemctl start mysql and mysql commands.

65:
Make sure the bash bin/install-wp-tests.sh script exists and is correctly configured.

115:
Make sure the bash bin/build.sh script exists and is correctly configured.

118:
Make sure the ./github/actions/php-scoper/action.yml file exists and contains the necessary configuration.

120:
Ensure that the correct value is being passed to the scoper input.

123:
Make sure the bash bin/clean.sh script exists and is correctly configured.


<!-- This is an auto-generated comment by OpenAI -->



- name: Install WordPress
run: bash bin/install-wp-tests.sh wordpress root root 127.0.0.1:3306 ${{ matrix.wp }}
Comment on lines 63 to 64
Copy link

Choose a reason for hiding this comment

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

🤖 OpenAI

A step is added to install WordPress for testing purposes. No issues found.


Comment on lines 58 to 65
Copy link

Choose a reason for hiding this comment

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

🤖 OpenAI

Steps related to starting MySQL, installing WordPress, and running tests have been modified. There are no issues with these changes.

- name: Run test suite
- name: Run test suite with Vendor
if: 'no' == matrix.scoper
run: composer test

- name: Run test suite with PHP Scoper
if: 'yes' == matrix.scoper
run: composer test:scoper

lint:
name: PHP Syntax check
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3

- name: Setup PHP with composer v2
uses: shivammathur/setup-php@v2
with:
php-version: 7.4
tools: composer
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

- name: Validate composer.json and composer.lock
run: composer validate

- name: Install dependencies
run: composer install --prefer-dist --no-progress --no-suggest

- name: Check Lint
run: composer lint

assets:
name: Check Assets
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@master
- uses: actions/checkout@v3

- name: Install Node
uses: actions/setup-node@v3
Comment on lines 98 to 104
Copy link

@github-actions github-actions bot Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

🤖 OpenAI

A new job named "Check Assets" has been added to the workflow. It runs on ubuntu-latest and includes the following steps:

  • Line 78: The step "Install Node" uses the actions/setup-node@v3 action.

Expand All @@ -81,7 +113,7 @@ jobs:

status-check:
name: Status Check
needs: [ test, assets ]
needs: [ test, lint, assets ]
runs-on: ubuntu-latest
steps:
- name: Display Status
Expand All @@ -93,7 +125,7 @@ jobs:
if: contains(github.ref, 'tags/')
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@master
- uses: actions/checkout@v3

- name: Setup PHP with composer v2
uses: shivammathur/setup-php@v2
Comment on lines 125 to 131
Copy link

@github-actions github-actions bot Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

🤖 OpenAI

A new job named "Build on Tags" has been added to the workflow. It runs only if the GitHub ref contains 'tags/' and includes the following steps:

  • Line 104: The step "Setup PHP with composer v2" uses the shivammathur/setup-php@v2 action.

Expand All @@ -103,15 +135,13 @@ jobs:
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

Comment on lines 135 to 137
Copy link

Choose a reason for hiding this comment

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

🤖 OpenAI

A new step is added to build the package. No issues found.

- name: Build Package.
- name: Build Package
run: bash bin/build.sh

- name: Do PHP scoper
run: |
composer global require humbug/php-scoper
~/.composer/vendor/bin/php-scoper add-prefix --output-dir=vendor-prefixed --force
composer dump-autoload --working-dir vendor-prefixed --classmap-authoritative
composer help-scoper
- uses: ./.github/actions/php-scoper
name: Do PHP Scoper
with:
scoper: true

- name: Clean Unwanted files
run: bash bin/clean.sh
Copy link

Choose a reason for hiding this comment

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

🤖 OpenAI

Several steps have been added to the "Build on Tags" job. Here are the key changes:

  • Line 117: A new step named "Do PHP Scoper" has been added. It uses a custom action located at ./.github/actions/php-scoper and sets the scoper parameter to true.
  • Line 122: The step "Clean Unwanted files" now runs the command bash bin/clean.sh.

Overall, the changes in the workflow file seem fine. However, it's important to review the contents of the custom actions (./.github/actions/php-scoper) to ensure they align with the desired behavior.

Expand Down
18 changes: 12 additions & 6 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,23 @@
"minimum-stability": "stable",
"scripts": {
"test": "phpunit",
"test:scoper": "./vendor-prefixed/vendor/bin/phpunit",
"lint": "phpcs --standard=phpcs.ruleset.xml $(find ./ -name '*.php')",
"fix": "phpcbf --standard=phpcs.ruleset.xml $(find ./ -name '*.php')",
"scoper": "php-scoper",
"prefix": [
"composer install --no-dev",
"scoper": [
"php-scoper add-prefix --output-dir=vendor-prefixed --force",
"composer dump-autoload --working-dir vendor-prefixed --classmap-authoritative",
"composer install",
"@help-scoper"
"@scoper-patch"
],
"scoper-patch": [
"php bin/patcher.php",
"chmod +x vendor-prefixed/vendor/bin/*"
],
"help-scoper": "php bin/patcher.php"
"prefix": [
"composer install --no-dev",
"@scoper",
"composer install"
]
},
"require": {
"php": ">=7.2",
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
"update": "wp-env start --update",
"stop": "wp-env stop",
"test": "wp-env run tests-cli ./wp-content/plugins/ga-communicator/vendor/bin/phpunit -c ./wp-content/plugins/ga-communicator/phpunit.xml.dist",
"test:scoper": "wp-env run tests-cli ./wp-content/plugins/ga-communicator/vendor-prefixed/vendor/bin/phpunit -c ./wp-content/plugins/ga-communicator/phpunit.xml.dist",
"env": "wp-env",
"cli": "wp-env run cli wp",
"cli:test": "wp-env run tests-cli wp",
Expand Down
27 changes: 15 additions & 12 deletions scoper.inc.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,6 @@
] ),
],

// List of excluded files, i.e. files for which the content will be left untouched.
// Paths are relative to the configuration file unless if they are already absolute
//
// For more see: https://github.com/humbug/php-scoper/blob/master/docs/configuration.md#patchers
'exclude-files' => [
'src/a-whitelisted-file.php',
],

// When scoping PHP files, there will be scenarios where some of the code being scoped indirectly references the
// original namespace. These will include, for example, strings or string manipulations. PHP-Scoper has limited
// support for prefixing such strings. To circumvent that, you can define patchers to manipulate the file to your
Comment on lines 36 to 41
Copy link

Choose a reason for hiding this comment

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

🤖 OpenAI

The comment in the new hunk provides a good explanation of the purpose of the patchers. However, it seems that the old hunk is missing some lines of code that were removed. Without the complete context, it's difficult to determine if there are any issues with this change.

Expand All @@ -60,14 +52,25 @@ static function ( string $filePath, string $prefix, string $contents ): string {

// For more information see: https://github.com/humbug/php-scoper/blob/master/docs/configuration.md#excluded-symbols
'exclude-namespaces' => [
'/^cli/',
'/^Composer/',
'/^DeepCopy/',
'/^Doctrine/',
'/^Kunoichi/',
'/^PharIo/',
'/^PHP_CodeSniffer/',
'/^PHPCompatibility/',
'/^PHPCSStandards/',
'/^PHPUnit/',
'/^Yoast/',
'/^SebastianBergmann/',
'/^TheSeer/',
'/^WP_/',
'/^Composer/',
'/^cli/'
],
Comment on lines 52 to 69
Copy link

Choose a reason for hiding this comment

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

🤖 OpenAI

The exclude-namespaces array in the PHP-Scoper configuration has been modified. Several namespaces have been added to exclude from scoping. However, it seems that some of the namespaces were already present in the original configuration. Please verify if these additions are intentional and necessary.

'exclude-classes' => [
'/^WP_/',
'/^Composer/'
'/^Composer/',
'/^PHPUnit/',
],
Comment on lines 70 to 74
Copy link

Choose a reason for hiding this comment

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

🤖 OpenAI

The exclude-classes array in the PHP-Scoper configuration has been modified. Some classes have been added to exclude from scoping. Similar to the previous comment, please ensure that these additions are intentional and necessary.

'exclude-functions' => [
// 'mb_str_split',
Comment on lines 52 to 76
Copy link

Choose a reason for hiding this comment

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

🤖 OpenAI

The new hunk updates the excluded namespaces, classes, and functions in the PHP-Scoper configuration file. It appears that the changes are intended to exclude additional namespaces, classes, and functions from being scoped. Without the complete context, it's difficult to determine if there are any issues with this change.

Expand All @@ -80,7 +83,7 @@ static function ( string $filePath, string $prefix, string $contents ): string {
//
// For more information see: https://github.com/humbug/php-scoper/blob/master/docs/configuration.md#exposed-symbols
'expose-global-constants' => true,
'expose-global-classes' => true,
'expose-global-classes' => false,
'expose-global-functions' => false,
'expose-namespaces' => [
],
Comment on lines 83 to 89
Copy link

@github-actions github-actions bot Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

🤖 OpenAI

The new hunk modifies the values of 'expose-global-classes' and 'expose-global-functions' in the PHP-Scoper configuration file. The value of 'expose-global-classes' has been changed to false, and 'expose-global-functions' remains false. Without the complete context, it's difficult to determine if there are any issues with this change.

Expand Down
8 changes: 7 additions & 1 deletion tests/bootstrap.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,13 @@
// Give access to tests_add_filter() function.
require_once $_tests_dir . '/includes/functions.php';

Comment on lines 19 to 21
Copy link

Choose a reason for hiding this comment

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

🤖 OpenAI

The comment on line 19 is unnecessary and can be removed.

require_once dirname( __DIR__ ) . '/vendor/autoload.php';
/**
* Manually load the plugin being tested.
*/
function _manually_load_plugin() {
require dirname( dirname( __FILE__ ) ) . '/ga-communicator.php';
}
Comment on lines +25 to +27
Copy link

Choose a reason for hiding this comment

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

🤖 OpenAI

The function _manually_load_plugin() is added to manually load the plugin being tested. This function should be documented with a PHPDoc block to describe its purpose, parameters, and return value.

/**
 * Manually load the plugin being tested.
 */
function _manually_load_plugin() {
    require dirname(dirname(__FILE__)) . '/ga-communicator.php';
}

28:
The tests_add_filter() function is used to add a filter to load the plugin before running the tests. It would be helpful to provide a comment explaining the purpose of this filter.

// Load the plugin before running the tests.
tests_add_filter('muplugins_loaded', '_manually_load_plugin');

Please ensure that the code snippets provided above are placed in the correct location within the file.

tests_add_filter( 'muplugins_loaded', '_manually_load_plugin' );

// Start up the WP testing environment.
require $_tests_dir . '/includes/bootstrap.php';
Loading