Update CI and fix various issues #39
Conversation
|
Warning Rate limit exceeded@alistair3149 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 31 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes update the continuous integration setup by modifying the CI workflow to adjust matrix configurations, add new static analysis jobs, and update cache and tool configurations. New configuration files for PHP CodeSniffer, PHPStan, and PHPUnit have been introduced alongside a Makefile that defines various testing and linting commands. Platform requirements in the README and dependency specifications in composer.json have been updated. Additionally, the source code has been refactored to improve type safety, including method signature adjustments and property type declarations, as well as consolidating and renaming RDF builder classes. Minor test and formatting improvements were also made. Changes
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: 2
🧹 Nitpick comments (5)
tests/php/Unit/LocalMediaRdfBuilderTest.php (1)
25-48: Consider updating assertions to verify more specific behavior.The test assertions are very basic, only checking that certain strings appear in the output. Consider enhancing these assertions to verify the exact RDF triple structure for more robust testing.
- $this->assertStringContainsString( 'File:Bunny.jpg', $rdf ); - $this->assertStringContainsString( $GLOBALS['wgServer'], $rdf ); + // Verify the exact RDF triple structure + $this->assertStringContainsString( '<http://www/Q42> <http://acme/testing> ', $rdf ); + $this->assertRegExp( '/File:Bunny\.jpg.*' . preg_quote($GLOBALS['wgServer'], '/') . '/', $rdf ); + // Verify the correct vocabulary and format is used + $this->assertStringContainsString( 'http://acme/testing', $rdf );phpstan-baseline.neon (1)
1-91: Good use of PHPStan baseline to address legacy issuesThis new configuration file defines PHPStan errors that should be ignored during static analysis. This is a pragmatic approach when introducing static analysis to an existing codebase, allowing gradual improvement while documenting known issues.
Many of the issues relate to missing type hints for arrays and parameters, which aligns with the PR objective of enhancing type safety. The baseline provides a clear path for future improvements by explicitly documenting each issue.
As the codebase continues to improve its type safety, you should aim to progressively reduce this baseline by addressing the listed issues in subsequent PRs.
I recommend setting up a process to periodically review and reduce this baseline file, perhaps tracking improvement metrics over time as part of your technical debt management.
src/Services/ImageLinkFormatter.php (1)
33-36:Details
❓ Verification inconclusive
Redundant type check in format method
The type check
if ( !( $value instanceof StringValue ) )will always evaluate to true or false in a predictable way since the parameter is already typed in the PHPDoc.Since you're adding type hints throughout the codebase, consider updating the method signature to include the parameter type:
- public function format( $value ) { + public function format( StringValue $value ) { - if ( !( $value instanceof StringValue ) ) { - throw new InvalidArgumentException( 'Data value type mismatch. Expected a StringValue.' ); - }This would make the type check unnecessary. However, if you need to maintain backward compatibility with the
ValueFormatterinterface, you may need to keep the runtime check.
🏁 Script executed:
#!/bin/bash # Check ValueFormatter interface definition to see if we can change the parameter type rg -A 10 "interface ValueFormatter" --glob "*.php"Length of output: 52
Attention: Further Verification Needed on the ValueFormatter Interface Contract
The automated search for an explicit definition of the
ValueFormatterinterface did not return any output. Without clear evidence of its definition—which would indicate whether the interface enforces a specific parameter type—we cannot conclusively determine if updating the method signature insrc/Services/ImageLinkFormatter.phpto use a type hint (i.e. changing frompublic function format( $value )topublic function format( StringValue $value )) would break the interface contract.
- Action Required:
• Please manually verify if theValueFormatterinterface enforces a particular signature or if it allows for type hints.
• If allowed, update the method signature to include theStringValuetype, which will eliminate the need for the runtime type-check.
• Otherwise, retain the runtime type-check for backward compatibility.For additional clarity, you might consider re-running a broader search in the repository with:
#!/bin/bash # Search for the definition of ValueFormatter throughout the repository with context rg -n -A 5 "interface\s+ValueFormatter"Once you confirm the contract defined by the interface, proceed accordingly.
Makefile (1)
7-12: Consider making phpunit path more configurable.The phpunit command uses a hardcoded path to the phpunit.php script, which might make the Makefile less portable across different MediaWiki installations.
Consider using a variable for the phpunit path:
- php ../../tests/phpunit/phpunit.php -c phpunit.xml.dist --filter $(filter) + php $(MW_PHPUNIT_PATH)/phpunit.php -c phpunit.xml.dist --filter $(filter)With a default definition at the top:
MW_PHPUNIT_PATH ?= ../../tests/phpunitsrc/WikibaseLocalMedia.php (1)
32-32: Reordered method modifiers for consistency.The order of modifiers has been changed from
protected finaltofinal protected. This is a minor stylistic change that improves consistency but doesn't affect functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.github/workflows/ci.yml(4 hunks)Makefile(1 hunks)README.md(1 hunks)composer.json(1 hunks)extension.json(2 hunks)phpcs.xml(1 hunks)phpstan-baseline.neon(1 hunks)phpstan.neon(1 hunks)phpunit.xml.dist(1 hunks)src/HookHandlers.php(2 hunks)src/Services/FormatterBuilder.php(2 hunks)src/Services/ImageLinkFormatter.php(1 hunks)src/Services/InlineImageFormatter.php(4 hunks)src/Services/LocalImageLinker.php(0 hunks)src/Services/LocalMediaRdfBuilder.php(1 hunks)src/Services/LocalMediaRdfBuilder34.php(0 hunks)src/WikibaseLocalMedia.php(3 hunks)tests/php/Integration/FormattingTest.php(1 hunks)tests/php/Integration/ValidationTest.php(2 hunks)tests/php/Unit/ImageLinkFormatterTest.php(1 hunks)tests/php/Unit/LocalMediaRdfBuilderTest.php(1 hunks)
💤 Files with no reviewable changes (2)
- src/Services/LocalImageLinker.php
- src/Services/LocalMediaRdfBuilder34.php
✅ Files skipped from review due to trivial changes (4)
- phpunit.xml.dist
- tests/php/Integration/FormattingTest.php
- tests/php/Unit/ImageLinkFormatterTest.php
- phpcs.xml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/ci.yml
[error] 204-204: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (42)
extension.json (2)
5-5: Major version increment indicates significant changes.The version has been incremented from 1.1.0 to 2.0.0, which correctly follows semantic versioning principles for the breaking changes represented by the updated requirements.
19-19: MediaWiki dependency has been updated appropriately.The MediaWiki requirement change from ">= 1.34.0" to ">= 1.39.0" aligns with the PR objectives and reflects the minimum version needed for modern features.
README.md (1)
25-27: Dependency requirements have been consistently updated.The platform requirements have been appropriately updated to match the changes in extension.json and composer.json:
- PHP 7.4+ requirement (up from 7.2)
- MediaWiki 1.39+ (updated from 1.34-1.39 range)
- Wikibase Repository REL1_39+ (simplified from listing multiple versions)
These changes provide clear guidance to users about compatibility.
phpstan.neon (2)
1-14: PHPStan configuration looks well-structured.This new configuration file properly sets up PHPStan for static analysis:
- Uses level 9 (highest strictness), which shows commitment to code quality
- Properly references MediaWiki core and Wikibase paths for analysis
- Includes the MediaWiki autoloader for proper class resolution
- References a baseline file to track and manage existing issues
This addition directly supports the PR objective of integrating PHPStan into the CI pipeline.
2-2:Details
✅ Verification successful
Verify the phpstan-baseline.neon file exists.
The configuration includes a reference to
phpstan-baseline.neon, but this file doesn't appear in the PR files. Ensure this file exists or has been created.
🏁 Script executed:
#!/bin/bash # Check if the phpstan-baseline.neon file exists if [ -f "phpstan-baseline.neon" ]; then echo "phpstan-baseline.neon exists" else echo "phpstan-baseline.neon does not exist" # Check if there are any other baseline files that might be misnamed find . -name "*phpstan*baseline*.neon" -type f fiLength of output: 101
PHPStan baseline file verified
The reference to
phpstan-baseline.neonhas been checked and confirmed to exist in the repository, so no further action is needed.tests/php/Unit/LocalMediaRdfBuilderTest.php (3)
13-13: Class import updated for consolidated RdfBuilder.The import statement has been correctly updated to use the new consolidated
LocalMediaRdfBuilderclass.
18-18: Documentation annotation updated consistently.The
@coversannotation has been properly updated to reflect the new class namespace path.
26-26: Builder instantiation updated correctly.The test now instantiates the consolidated
LocalMediaRdfBuilderclass and passes the TitleFactory dependency properly.src/Services/LocalMediaRdfBuilder.php (3)
11-11: Appropriate class renaming to remove version suffixRemoving the version-specific suffix from the class name (likely from
LocalMediaRdfBuilder35) improves code maintenance and follows better naming practices.
13-13: Good addition of property type declarationAdding the
TitleFactorytype to the property improves type safety and code clarity.
24-26:Details
❓ Verification inconclusive
Potential null dereference in getValueUri method
The
newFromText()method can return null, but the code assumes it always returns a non-null Title object by directly callinggetFullURL()on the result.Based on the phpstan-baseline entries, this is a known issue. Consider refactoring to handle the potential null return:
protected function getValueUri( DataValue $value ) { - return $this->titleFactory->newFromText( $value->getValue(), NS_FILE )->getFullURL(); + $title = $this->titleFactory->newFromText( $value->getValue(), NS_FILE ); + return $title ? $title->getFullURL() : ''; }
🏁 Script executed:
#!/bin/bash # Check if newFromText is documented to return null in some cases rg -A 2 -B 2 "function newFromText" --glob "*.php"Length of output: 52
Action Required: Manually Verify Handling of Potential Null from newFromText
The static analysis via phpstan-baseline suggests that
newFromText()may return null. Although our initial search didn’t locate its definition, this may be due to an unconventional implementation or external dependency. Please verify that the Title object returned bynewFromText()cannot be assumed non-null. In the meantime, consider refactoring as follows:protected function getValueUri( DataValue $value ) { - return $this->titleFactory->newFromText( $value->getValue(), NS_FILE )->getFullURL(); + $title = $this->titleFactory->newFromText( $value->getValue(), NS_FILE ); + return $title ? $title->getFullURL() : ''; }
- Next Steps:
- Confirm in the TitleFactory (or equivalent) implementation whether
newFromText()can indeed return null.- Ensure any calling code accounts for an empty URI when no valid Title is produced.
src/Services/ImageLinkFormatter.php (1)
15-16: Good addition of property type declarationsAdding proper type declarations for
$imageLinkerand$cssClassimproves type safety and code clarity. This aligns well with the PR objectives of enhancing type hints.tests/php/Integration/ValidationTest.php (2)
21-22: Improved assertion specificityChanging from
assertEmpty()toassertCount(0, ...)makes the test's intent more explicit. It clearly communicates that we're expecting zero validation errors rather than just an empty result.
33-33: Improved code formattingThe removal of extra spaces around the variable declaration in the
foreachloop improves code consistency.composer.json (3)
47-47: Appropriately increased PHP version requirement.Increased PHP version requirement from >=7.2 to >=7.4, which aligns with the PR objectives to update the PHP version requirements. This ensures better compatibility with modern PHP features and security updates.
51-52: Good addition of development tools for code quality.The changes to the development dependencies are positive:
- PHPStan updated to a specific version (^2.1.6) instead of dev-master
- Added MediaWiki CodeSniffer for enforcing coding standards
These additions align with the PR objectives to integrate PHPStan and PHPCS into the CI pipeline.
56-58: Proper configuration for CodeSniffer composer installer.The addition of the dealerdirect/phpcodesniffer-composer-installer to allowed plugins is necessary for the CodeSniffer to work properly with Composer's plugin system. This ensures that the coding standards are correctly installed and available.
Makefile (4)
3-6: Well-structured CI command organization.Good organization of CI commands that clearly shows the workflow: running tests and code style checks. This provides an easy-to-understand entry point for development tasks.
17-19: Clean phpcs command with proper path resolution.The phpcs command correctly uses the shell's pwd to ensure the path to phpcs.xml is always correct, making it more robust.
20-22: PHPStan configuration looks good.The PHPStan command is properly configured with the right parameters for analysis, including memory limit which is essential for larger projects.
30-31:Details
❓ Verification inconclusive
Verify Node.js version compatibility.
Using Node.js 22 in the Docker command might lead to compatibility issues as it's a very recent version. Node.js 20 LTS might be more stable for CI environments.
🏁 Script executed:
#!/bin/bash # Check if Node.js 22 is actually necessary by examining any Node.js dependencies # Look for package.json and check its engines field if [ -f package.json ]; then echo "Checking Node.js version requirements in package.json:" grep -A 5 '"engines"' package.json || echo "No engines field found" fi # Check if there are any packages that might have issues with Node 22 if [ -f package-lock.json ]; then echo "Packages that might have Node.js version constraints:" grep -A 3 '"engines"' package-lock.json | grep -B 1 "node" || echo "No explicit Node.js constraints found in dependencies" fiLength of output: 56
Attention: Node.js Version Compatibility Verification Required
The Makefile's Docker command currently uses the Node.js 22 image. Our automated check of the repository's package.json and package-lock.json did not yield any explicit Node.js version requirements. However, since no output was produced from those checks, it's unclear if Node.js 22 is genuinely compatible with all dependencies or if switching to the more stable Node.js 20 LTS could be beneficial for CI purposes.
- File: Makefile, Lines 30-31
- Action: Manually verify if Node.js 22 is required by your project or consider testing with Node.js 20 LTS in your CI environment for increased stability.
.github/workflows/ci.yml (4)
15-15: Good use of experimental flag for conditional error handling.Setting
continue-on-errorbased on the experimental flag is a good approach that allows testing with cutting-edge PHP versions without breaking the entire workflow when these experimental builds fail.
20-38: Well-structured test matrix with appropriate PHP and MediaWiki versions.The test matrix has been well organized to cover:
- Multiple MediaWiki versions from 1.39 to 1.43
- Appropriate PHP versions for each MediaWiki version
- Experimental testing with PHP 8.4 and the master branch
This comprehensive testing strategy ensures compatibility across a wide range of environments.
57-58: Updated cache actions to the latest version.Cache actions have been updated from v2 to v4, which likely brings performance improvements and bug fixes to the caching mechanism.
Also applies to: 66-67
90-152: Well-structured PHPStan job.The PHPStan job is properly configured to:
- Run against a specific MediaWiki and PHP version
- Utilize caching for better performance
- Output results in a format that can be displayed in GitHub
- Properly handle Composer dependencies
This aligns well with the PR objective to integrate PHPStan into the CI pipeline.
src/Services/FormatterBuilder.php (2)
17-18: Improved type safety with specific property types.Adding explicit type declarations to the class properties improves type safety and code clarity:
$formatterBuildersis now explicitly typed asWikibaseValueFormatterBuilders$thumbLimitsis explicitly typed asarrayThis change aligns with the PR objective to enhance type safety.
44-44:Details
❓ Verification inconclusive
Verify parameter removal from newStringFormatter method call.
The
$optionsparameter has been removed from the call tonewStringFormatter. This change needs verification to ensure it doesn't break functionality.If the method signature in
WikibaseValueFormatterBuildersonly expects one parameter, this change is correct. If it expects and uses two parameters, this could result in functionality loss.
🏁 Script executed:
#!/bin/bash # Check the method signature of newStringFormatter in WikibaseValueFormatterBuilders # Look for the method definition echo "Searching for the newStringFormatter method definition:" rg -A 10 "function\s+newStringFormatter" --type php # Check if there are any calls that still pass options echo "Checking for other calls to newStringFormatter that might pass options:" rg "newStringFormatter\s*\(\s*[^,)]+\s*,\s*" --type phpLength of output: 385
Action Required: Manually Verify the
newStringFormatterParameter ChangeBased on the automated search:
- No occurrences were found where
newStringFormatteris called with more than one parameter.- The automated search did not return any output for the method signature in
WikibaseValueFormatterBuilders, so we couldn’t conclusively verify whether the method now only expects a single parameter.Next Steps:
- Manually check in
WikibaseValueFormatterBuildersthat thenewStringFormattermethod’s signature accepts only one parameter.- Confirm that the removal of the
$optionsparameter in the call fromsrc/Services/FormatterBuilder.php(line 44) does not break any functionality.This review comment remains under verification. Please ensure the method definition is reviewed and validated manually.
src/HookHandlers.php (6)
8-8: Type import added for clarity and type safety.The explicit import of
ValueFormatters\FormatterOptionsaligns with the PR objective of enhancing type safety and makes the code more self-documenting.
16-18: Anonymous function converted to static closure.Converting to static function closures is a good practice as it prevents capturing the
$thiscontext when it's not needed, resulting in better memory usage and performance.
19-22: Improved callback type safety with static function.The conversion to a static function with explicit type hints for parameters enhances type safety while improving performance by avoiding the capture of unnecessary context.
23-25: Callback converted to static function.Similar to other callbacks in this class, the static keyword has been added for consistency and improved memory usage.
26-33: Added version annotation and upgraded RDF callback.The
@since MediaWiki 1.37annotation provides useful information about compatibility. The static keyword addition is consistent with the other callbacks in this file.
40-43: Client data types formatter callback updated to static.This change is consistent with the repo data types callbacks, improving code quality and memory usage.
src/WikibaseLocalMedia.php (3)
10-10: Simplified RDF builder import.The import has been updated to use a single consolidated RDF builder class instead of multiple version-specific builder classes, which aligns with the code changes later in this file.
15-18: Added proper type annotation for static instance property.The PHPDoc
@var ?selfannotation properly documents that this property can be either null or an instance of the class, improving type safety and IDE support.
49-51: Simplified RDF builder implementation with return type.The method now has an explicit return type declaration and a simplified implementation that directly instantiates the consolidated RDF builder class. This removes conditional logic for different MediaWiki versions, making the code cleaner and easier to maintain.
src/Services/InlineImageFormatter.php (6)
25-26: Improved encapsulation by changing constant visibility.The
FALLBACK_THUMBNAIL_WIDTHconstant has been changed from public to private, which is a good practice as this constant is only used internally within the class. This prevents external code from depending on this implementation detail.
28-36: Added property type declarations for improved type safety.PHP 7.4+ property type declarations have been added to all class properties, which aligns with the PR objective of enhancing type safety. This makes the expected types explicit and helps catch type-related errors earlier.
103-105: Added parameter and return type declarations to method.Type declarations have been added to the
getThumbWidthmethod, making it clear that it accepts and returns integers. This improves type safety and makes the code more self-documenting.
107-118: Added parameter and return type declarations to wrapThumb method.The method signature now clearly indicates that it takes a Title object and a string, and returns a string. This improves type safety and code clarity.
120-135: Improved type safety with nullable parameter type.The
getCaptionHtmlmethod now explicitly declares that the$fileparameter can be null, which matches its existing usage in the code. This makes the intended behavior clearer and improves type safety.
137-142: Added return type and improved parameter type safety.The
getFileMetaHtmlmethod now explicitly returns a string, and on line 140, the file size is explicitly cast to an integer before being passed toformatSize(), ensuring type safety.
| /** @since MediaWiki 1.37 */ | ||
| if ( class_exists( 'Wikibase\Repo\Rdf\PropertySpecificComponentsRdfBuilder' ) ) { | ||
| return \Wikibase\Repo\Rdf\PropertySpecificComponentsRdfBuilder::OBJECT_PROPERTY; | ||
| } |
There was a problem hiding this comment.
If this is available since 1.37 and we're targetting 1.39, do we still need this check?
There was a problem hiding this comment.
Thanks for catching that, I'll follow up on that!
|
Does this fix these issues? |
Key changes:
Summary by CodeRabbit
New Features
Documentation
Refactor
Chores