Conversation
Replace standalone friendsofphp/php-cs-fixer and squizlabs/php_codesniffer with the shared IxDF coding standard package. Update .php-cs-fixer.php to use the shared Config factory and phpcs.xml to reference IxDFCodingStandard.
Auto-fixed violations detected by the new IxDF coding standard. Kept HttpClient and AbstractService::boot() non-final to preserve testability and extensibility.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request modernizes the project's coding standards by migrating to a unified Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the project to a new coding standard, primarily through automated style fixes, aiming for improved consistency and maintainability. However, critical PHP version compatibility and security issues have been introduced. The use of #[Override] (PHP 8.3+) and mb_trim (PHP 8.4+) in src/Support/HttpClient.php will cause fatal errors on the target PHP 8.2 environment, leading to a Denial of Service. Additionally, mb_substr with byte-based offsets from cURL could result in data corruption when multi-byte characters are present in HTTP responses. These incompatible features must be addressed to ensure proper functionality and security.
| #[\Override] | ||
| public function hydrate(array $attributes = []): Location | ||
| final public function hydrate(array $attributes = []): Location |
There was a problem hiding this comment.
The #[Override] attribute was introduced in PHP 8.3. Your rector.php configuration specifies PHP 8.2 compatibility, and your composer.json allows for PHP 8.2. Using this attribute will cause a fatal error (Attribute class "Override" not found) in a PHP 8.2 environment. To maintain compatibility, this attribute should be removed. This issue is present in multiple files across the pull request.
final public function hydrate(array $attributes = []): Location| #[\Override] | ||
| public function config(string $key, mixed $default = null) | ||
| final public function config(string $key, mixed $default = null) |
There was a problem hiding this comment.
Similar to the comment above, the #[Override] attribute is not compatible with PHP 8.2 and should be removed to prevent fatal errors and maintain the advertised version compatibility.
final public function config(string $key, mixed $default = null)| #[\Override] | ||
| protected static function getFacadeAccessor(): string |
There was a problem hiding this comment.
The #[Override] attribute is a PHP 8.3+ feature. As the project appears to support PHP 8.2, using this attribute will lead to a fatal error. It should be removed to ensure compatibility.
protected static function getFacadeAccessor(): string| $header = mb_substr($response, 0, $header_size); | ||
| $body = mb_substr($response, $header_size); |
There was a problem hiding this comment.
The use of mb_substr here is incorrect and can lead to data corruption. curl_getinfo($curl, \CURLINFO_HEADER_SIZE) returns the size of the headers in bytes, but mb_substr (when used with a multi-byte encoding like UTF-8) expects offsets and lengths in characters. If the HTTP response contains any multi-byte characters (e.g., in headers or at the beginning of the body), the split between headers and body will be calculated incorrectly, leading to corrupted data and subsequent failures in json_decode. Revert to using substr which operates on bytes.
$header = substr($response, 0, $header_size);
$body = substr($response, $header_size);|
|
||
| if (count($header) === 2) { | ||
| $result[$header[0]] = trim($header[1]); | ||
| $result[$header[0]] = mb_trim($header[1]); |
There was a problem hiding this comment.
The function mb_trim was introduced in PHP 8.4. Since this project targets PHP 8.2 (as specified in rector.php), calling mb_trim will result in a Fatal error: Call to undefined function mb_trim(), causing the application to crash whenever a GeoIP service response is parsed. This constitutes a Denial of Service (DoS) vulnerability. Revert to using trim.
$result[$header[0]] = trim($header[1]);
1 participant
Summary
friendsofphp/php-cs-fixerandsquizlabs/php_codesnifferwithinteraction-design-foundation/coding-standard^0.7.php-cs-fixer.phpto use the sharedConfig::create()factoryphpcs.xmlto reference theIxDFCodingStandardrulesetphpcbfandphp-cs-fixerHttpClientnon-final (mocked in tests) andAbstractService::boot()non-final (overridden by subclasses)Notes
Test plan
composer updatesucceedsvendor/bin/phpunitpasses (44 tests, 98 assertions)vendor/bin/php-cs-fixer fix --dry-runreports no fixable issues