HP-2116: Run pipelines on php-billing repo#77
Conversation
WalkthroughThe changes involve an update to the PHP version specified in the GitHub Actions workflows for running PHPUnit tests, changing from PHP 8.1 to PHP 8.3. The PHPUnit command has been modified to include options for generating code coverage reports. Additionally, new steps have been added to archive code coverage results using Codecov and to upload code coverage data to Scrutinizer. Overall, these updates enhance the testing process and integration with external services. Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant GitHubActions
participant PHPUnit
participant Codecov
participant Scrutinizer
Developer->>GitHubActions: Push changes
GitHubActions->>PHPUnit: Run tests on PHP 8.3
PHPUnit->>GitHubActions: Return test results
GitHubActions->>Codecov: Archive coverage results
GitHubActions->>Scrutinizer: Upload coverage data
GitHubActions-->>Developer: Send results
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
.github/workflows/phpunit-tests.yml (1)
14-14: Approve the PHP version update with additional considerations.Updating the PHP version to 8.3 in the test environment is a good change as it ensures the codebase is tested against the latest stable PHP version. PHP 8.3 introduces new features and improvements that could enhance the codebase.
However, please consider the following:
- Thoroughly test the codebase with PHP 8.3 to ensure there are no breaking changes or unexpected behavior.
- Update the development environment and production servers to PHP 8.3 to maintain consistency across all environments.
- Update the PHP version in other relevant configuration files such as
composer.json,Dockerfile, etc.- Communicate the PHP version update to the team and update the relevant documentation.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- .github/workflows/behat-tests.yml (1 hunks)
- .github/workflows/phpunit-tests.yml (1 hunks)
- composer.json (1 hunks)
Additional comments not posted (1)
composer.json (1)
46-46: Verify compatibility and update CI/CD pipeline.Updating the required PHP version to
^8.3aligns the project with the latest PHP standards and capabilities. However, please ensure that:
- All other dependencies in the project are compatible with PHP 8.3.
- The CI/CD pipeline is updated to use PHP 8.3 for testing and deployment.
Run the following script to verify the compatibility and CI/CD pipeline:
Verification successful
PHP 8.3 upgrade correctly implemented, but update testing tools
The PHP 8.3 requirement has been correctly updated in
composer.json, and no conflicting PHP version requirements were found in other files. However, to ensure full compatibility:
- Update Behat, PHPUnit, PHPStan, and Psalm to their latest versions that support PHP 8.3.
- Run your test suite and static analysis tools with PHP 8.3 to catch any potential compatibility issues.
- If you have a custom build or CI process, ensure it uses PHP 8.3 for all stages (build, test, static analysis).
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify compatibility and CI/CD pipeline for PHP 8.3 upgrade. # Test 1: Check if any other dependency in the `composer.json` file requires a PHP version lower than 8.3. # Expect: No dependencies requiring PHP version lower than 8.3. rg --type json $'"php":\s*"\^[0-7]\.[0-9]"' -g 'composer.json' # Test 2: Check if the CI/CD pipeline configuration files use PHP 8.3 for testing and deployment. # Expect: All pipeline configurations use PHP 8.3. rg --type yaml $'php:\s*[\'"]?8\.3' -g '.github/workflows/*.yml' rg --type yaml $'php:\s*[\'"]?8\.3' -g '.travis.yml' rg --type yaml $'php:\s*[\'"]?8\.3' -g '.circleci/config.yml' rg --type yaml $'php:\s*[\'"]?8\.3' -g 'bitbucket-pipelines.yml' rg --type yaml $'php:\s*[\'"]?8\.3' -g 'azure-pipelines.yml'Length of output: 970
Script:
#!/bin/bash # 1. Verify the existence and content of composer.json echo "=== composer.json content ===" cat composer.json # 2. List existing CI/CD configuration files echo -e "\n=== Existing CI/CD configuration files ===" fd -e yml -e yaml # 3. Check the content of existing CI/CD files for any PHP version specifications echo -e "\n=== PHP versions in CI/CD files ===" fd -e yml -e yaml -x grep -H -i 'php:' {} +Length of output: 3308
Script:
#!/bin/bash # Check content of behat.yml and hidev.yml for PHP version specifications echo "=== Content of behat.yml ===" cat behat.yml echo -e "\n=== Content of hidev.yml ===" cat hidev.yml # Search for any files containing CI/CD related keywords echo -e "\n=== Files potentially related to CI/CD ===" fd -t f | xargs grep -l -i -E 'ci|cd|test|build|deploy' # Search for PHP version specifications in all files echo -e "\n=== PHP version specifications in all files ===" fd -t f | xargs grep -H -i 'php:' || echo "No PHP version specifications found"Length of output: 4358
.github/workflows/behat-tests.yml
Outdated
| strategy: | ||
| matrix: | ||
| php: ['8.1'] | ||
| php: ['8.3'] |
There was a problem hiding this comment.
Verify PHP 8.3 compatibility before updating the version.
Updating the PHP version from 8.1 to 8.3 in the job matrix can potentially impact the compatibility and functionality of the tests being executed. PHP 8.3 may introduce new features, deprecations, or changes in behavior that could affect the codebase and its dependencies.
Before merging this change, please ensure that:
- The codebase is compatible with PHP 8.3 and does not rely on PHP 8.1 specific features.
- All dependencies are compatible with PHP 8.3. Update the dependencies if necessary.
- The tests pass successfully with PHP 8.3.
If the codebase requires significant changes to be compatible with PHP 8.3, consider a phased approach to update the PHP version. You can start by adding PHP 8.3 to the test matrix alongside PHP 8.1 to identify and fix compatibility issues gradually.
- php: ['8.3']
+ php: ['8.1', '8.3']Once the codebase and dependencies are fully compatible with PHP 8.3, you can remove PHP 8.1 from the matrix.
Committable suggestion was skipped due to low confidence.
…sts\unit\order\CalculatorTest::testCalculateCharges() must be compatible with hiqdev\php\billing\tests\unit\plan\PlanTest::testCalculateCharges(): void in /home/runner/work/php-billing/php-billing/tests/unit/order/CalculatorTest.php on line 57' error
|
Thank you. Could you fix Scrutinizer job as well, please? |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
src/price/Sums.php (1)
9-11: Please clarify the purpose of the$valuesparameter and update the documentation.The comments indicate that the purpose of the
$valuesparameter in the constructor is unclear. It would be helpful to provide more context on what the quantities and sums represent.Consider updating the documentation to clearly explain the meaning of the
$valuesparameter and how it is used within the class.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/price/Sums.php (2 hunks)
- tests/support/plan/CertificatePlan.php (1 hunks)
Additional comments not posted (2)
src/price/Sums.php (1)
36-39: LGTM!The new
getSum(int $quantity)method provides a convenient way to retrieve the sum for a specific quantity. The implementation is clear and concise, using the null coalescing operator to handle cases where the quantity does not exist in the$valuesarray.The addition of this method enhances the functionality of the
Sumsclass and improves its usability.tests/support/plan/CertificatePlan.php (1)
111-114: LGTM!The changes in the
getRawPricemethod look good:
- Renaming
$yearsto$quantityimproves clarity without affecting functionality.- Using
getSum($quantity)instead of directly indexing into the raw prices array allows for more flexible price retrieval based on the quantity.The changes align with the provided summary and do not introduce any apparent issues or inconsistencies.
…cordThat does not exist' Psalm error
…dons\DayPeriod::addTo has wrong name $since' Psalm error
…v\DataMapper\Query\Specification does not exist' Psalm error
…: UndefinedClass: Class, interface or enum named hiqdev\billing\hiapi\price\SubPrices does not exist' Psalm error
…: UndefinedClass: Class, interface or enum named hiqdev\billing\hiapi\price\SubPrices does not exist' Psalm error
…on depends on class or interface hiapi\exceptions\hiapiexception' Psalm error
…: UndefinedClass: Class, interface or enum named hiqdev\billing\hiapi\price\SubPrices does not exist' Psalm error
…plan\Plan::setParentId has wrong name $id, expecting $parentId' Psalm error
…price\SinglePrice::calculatePrice has wrong name $usage, expecting $quantity' Psalm error
…le: $actionKey is never referenced or the value is not used' Psalm error
…::offsetGet does not exist' Psalm error
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
src/price/SinglePrice.php (1)
66-66: LGTM!The change in the method signature does not introduce any issues and the functionality remains unaffected.
Consider renaming the method to better align with its functionality. For example,
getPrice()might be more appropriate since the method always returns the same price regardless of the input quantity.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- composer.json (2 hunks)
- src/order/Calculator.php (4 hunks)
- src/plan/Plan.php (1 hunks)
- src/price/PriceInvalidArgumentException.php (1 hunks)
- src/price/PriceWithSubpriceInterface.php (0 hunks)
- src/price/SinglePrice.php (1 hunks)
- src/sale/SaleRepositoryInterface.php (1 hunks)
Files not reviewed due to no reviewable changes (1)
- src/price/PriceWithSubpriceInterface.php
Files skipped from review as they are similar to previous changes (1)
- composer.json
Additional comments not posted (7)
src/price/PriceInvalidArgumentException.php (1)
5-5: Verify the impact of changing the base exception class.The
PriceInvalidArgumentExceptionclass now extendsRuntimeExceptioninstead ofHiapiException. This change in the base exception class could potentially affect exception handling logic in other parts of the codebase.Please run the following script to verify the usage of
PriceInvalidArgumentExceptionandHiapiExceptionacross the codebase:Review the results to ensure that:
- Exception handling code is updated to catch
RuntimeExceptioninstead ofHiapiExceptionwherePriceInvalidArgumentExceptionis used.- Any specific functionality or properties of
HiapiExceptionthat were being used withPriceInvalidArgumentExceptionare refactored appropriately.If you find any inconsistencies or areas that need updating, please let me know, and I'll be happy to suggest the necessary changes.
Also applies to: 7-7
Verification successful
Change to RuntimeException appears to have minimal impact
The switch from
HiapiExceptiontoRuntimeExceptionas the base class forPriceInvalidArgumentExceptionseems to have minimal impact on the codebase. The exception is used insrc/price/Sums.phpandsrc/price/SumsHydrator.php, but only for throwing, not catching. No usage ofHiapiExceptionwas found.While this change simplifies the exception hierarchy, please verify:
- Any global exception handling that might have been catching
HiapiException(if it existed) is updated to handleRuntimeExceptionif necessary.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash echo "Searching for usage of PriceInvalidArgumentException..." rg --type php -w 'PriceInvalidArgumentException' -A 5 echo "Searching for usage of HiapiException..." rg --type php -w 'HiapiException' -A 5Length of output: 1077
src/sale/SaleRepositoryInterface.php (1)
37-37: Verify the impact of the return type change.The return type annotation change from
PlanInterface[]toSaleInterface[]improves code clarity and consistency within theSaleRepositoryInterface.Please ensure that:
- Existing implementations of this interface are updated to return
SaleInterfaceobjects.- Dependent code that relies on the returned
PlanInterfaceobjects is refactored to handleSaleInterfaceobjects.Run the following script to verify the impact:
Verification successful
To gather more information about the implementation details and potential impacts, let's run the following scripts:
Return type change is verified and approved.
The return type annotation change from
PlanInterface[]toSaleInterface[]inSaleRepositoryInterface::findByOrderhas been successfully implemented and its impact has been verified. Here are the key findings:
- The
SimpleSaleRepositoryimplementation is compatible with the new return type, as it returns an array of sales without explicit type hints.- The
Calculatorclass, which usesfindByOrder, remains compatible as it treats the result as an iterable without type-checking.- The docblock in
SaleRepositoryInterfacehas been correctly updated to reflect the new return type.- No code changes are required in the existing implementation due to the absence of explicit return type hints.
Note: The
SimplePlanRepositoryandPlanRepositoryInterfaceare separate fromSaleRepositoryInterfaceand are not affected by this change.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the return type change in `SaleRepositoryInterface::findByOrder`. # Test 1: Search for implementations of the `SaleRepositoryInterface`. # Expect: Implementations should return `SaleInterface` objects. rg --type php -A 5 $'implements SaleRepositoryInterface' # Test 2: Search for usages of the `findByOrder` method. # Expect: Dependent code should handle `SaleInterface` objects. rg --type php -A 5 $'findByOrder'Length of output: 2737
Script:
#!/bin/bash # Description: Check implementation details and potential impacts of the return type change # Check the implementation of SimpleSaleRepository::findByOrder echo "Checking SimpleSaleRepository::findByOrder implementation:" rg --type php -A 10 "public function findByOrder" tests/support/sale/SimpleSaleRepository.php # Check if SimplePlanRepository implements SaleRepositoryInterface echo "\nChecking if SimplePlanRepository implements SaleRepositoryInterface:" rg --type php "class SimplePlanRepository" tests/support/plan/SimplePlanRepository.php # Check for type hints or docblocks in findByOrder implementations and usages echo "\nChecking for type hints or docblocks in findByOrder implementations and usages:" rg --type php -A 5 -B 3 "function findByOrder|->findByOrder"Length of output: 4316
src/plan/Plan.php (1)
112-114: LGTM!The parameter name change from
$idto$parentIdenhances code readability by clearly indicating the purpose of the parameter. The functionality of the method remains unchanged.src/order/Calculator.php (4)
64-69: LGTM!The simplification of the condition checking for the existence of plans associated with actions improves readability and maintains the correct logic. The error handling remains intact, ensuring that exceptions are thrown for any issues encountered during the calculation of plans.
156-158: LGTM!Explicitly defining the return type as an array in the method signature of
findPlansenhances type clarity and improves the code's readability and maintainability.
Line range hint
164-182: LGTM!The adjustment to the logic in the
findPlansmethod simplifies the code and improves readability. By directly checking for non-empty sales instead of comparing againstfalse, the assignment ofnullto plans when no sales are found becomes more straightforward and easier to understand. The updated comments also provide better clarity and context.
203-206: LGTM!Explicitly defining the return type as an array in the method signature of
findSalesenhances type clarity and improves the code's readability and maintainability.
…charge\modifiers\AddonInterface::isAbsolute' Psalm error
…::getExpression does not exist' Psalm error
…::getExpression does not exist' Psalm error
…::getExpression does not exist' Psalm error
|
I replaced Scrutinizer with Psalm and fixed all the errors that occurred. After that, I removed the .scrutinizer.yml file, but Scrutinizer keeps launching. |
| * Quantity of what? | ||
| * Sum of what? | ||
| * If you know answers please write in the comment |
There was a problem hiding this comment.
Quantity of what?
Of the action being charges
Sum of what?
Amount for this given Quantity
If used to denote bulk prices.
E.g. when you buy an SSL certificate for 1 year – it costs 10$
But for 2 years you pay 15$.
It is stored as
[ 1 => 10, 2 => 15 ]
| * What purpose of this method? Because it looks like duplicate of PriceHydrator::extract() | ||
| * Where we are using the result of this method? | ||
| * Magic calls can't be determined and I don't know what can be broken if we change the method result. | ||
| * Which structure must have the result, because array can contain anything? |
There was a problem hiding this comment.
What purpose of this method? Because it looks like duplicate of PriceHydrator::extract()
PriceHydrator::extract() is highly-bound to HiAPI context and public API. This method provides a naive way to encode all the properties.
Where we are using the result of this method?
I can not remember the use cases. Maybe is not used at all not.
Add a logging statement with the stacktace, run tests and see when this method is being called.
|
Remove the Scrutinizer workflow, please |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores