Improve compatibility with recent MediaWiki versions #189
Conversation
alistair3149
changed the title
alistair3149
force-pushed
the
update-ci
branch
2 times, most recently
from
d78d517 to
7b0e6b4
Compare
alistair3149
changed the title
📝 WalkthroughWalkthroughThis pull request implements changes across configuration, localization, source, and test files. The CI workflow has been updated to refine job definitions, matrix configurations, caching, and error handling. Dependency declarations and extension metadata have been revised in composer and extension files. All internationalization files have been updated to reference a new configuration page. Additionally, static analysis settings were refined, source code was refactored for clarity and robustness, and several test files were modified to simplify logic, update signatures, and remove obsolete code. Changes
✨ Finishing Touches
🪧 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: 0
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
153-157: Trailing Spaces Cleanup in Composer Step
There are extraneous trailing spaces detected on lines 155 and 157 in the Composer configuration step. Removing these will clean up the YAML formatting.Proposed diff to remove trailing spaces:
- run: composer config --no-plugins allow-plugins.composer/installers true␣␣ - - - run: composer update␣␣ + run: composer config --no-plugins allow-plugins.composer/installers true + - run: composer update🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 155-155: trailing spaces
(trailing-spaces)
[error] 157-157: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
-
.github/workflows/ci.yml(9 hunks) -
WikibaseExport.alias.php(1 hunks) -
composer.json(2 hunks) -
extension.json(4 hunks) -
i18n/ar.json(2 hunks) -
i18n/de.json(2 hunks) -
i18n/en.json(2 hunks) -
i18n/fr.json(2 hunks) -
i18n/he.json(2 hunks) -
i18n/ia.json(2 hunks) -
i18n/id.json(2 hunks) -
i18n/mk.json(2 hunks) -
i18n/nl.json(2 hunks) -
i18n/pt-br.json(1 hunks) -
i18n/pt.json(2 hunks) -
i18n/qqq.json(1 hunks) -
i18n/sl.json(2 hunks) -
i18n/tt-cyrl.json(1 hunks) -
i18n/vi.json(2 hunks) -
i18n/zh-hans.json(2 hunks) -
i18n/zh-hant.json(2 hunks) -
phpstan-baseline.neon(0 hunks) -
phpstan.neon(1 hunks) -
resources/ext.wikibase.export.js(1 hunks) -
src/Application/SearchEntities/SearchEntitiesUseCase.php(4 hunks) -
src/EntryPoints/SpecialWikibaseExport.php(2 hunks) -
src/EntryPoints/SpecialWikibaseExportConfig.php(0 hunks) -
tests/Application/SearchEntities/SearchEntitiesUseCaseTest.php(2 hunks) -
tests/EntryPoints/SearchEntitiesApiTest.php(1 hunks) -
tests/Presentation/ConfigPageSmokeTest.php(2 hunks) -
tests/TestDoubles/StubEntitySearchHelper.php(1 hunks) -
tests/TestDoubles/StubEntitySearchHelper39.php(0 hunks) -
tests/WikibaseExportIntegrationTest.php(2 hunks)
💤 Files with no reviewable changes (3)
- phpstan-baseline.neon
- src/EntryPoints/SpecialWikibaseExportConfig.php
- tests/TestDoubles/StubEntitySearchHelper39.php
✅ Files skipped from review due to trivial changes (7)
- i18n/de.json
- i18n/ia.json
- i18n/id.json
- i18n/pt.json
- i18n/qqq.json
- i18n/fr.json
- i18n/mk.json
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/ci.yml
[error] 155-155: trailing spaces
(trailing-spaces)
[error] 157-157: trailing spaces
(trailing-spaces)
🔇 Additional comments (53)
composer.json (2)
27-27: Updated Psalm dependency as specified in PR objectivesThe Psalm version has been upgraded from ^4.22.0 to ^4.30.0 as intended, which should provide improved static analysis capabilities.
36-38: Added CodeSniffer installer plugin to allowed pluginsGood addition of the phpcodesniffer-composer-installer plugin, which will automatically install coding standards and make it easier to maintain consistent code style.
i18n/tt-cyrl.json (1)
8-8: Link path update matches PR objectiveThe admin notice now correctly references
MediaWiki:WikibaseExportinstead of the outdatedSpecial:WikibaseExportConfigpage. This change aligns with the PR objective of removing the deprecated special page.tests/WikibaseExportIntegrationTest.php (2)
8-8: Added necessary imports for new functionalityImporting the Title class from the MediaWiki core to enable title manipulation in the new deleteConfigPage method.
Also applies to: 16-16
32-39: Good addition of cleanup method for configuration pageThis method provides a clean way to delete the configuration page during tests, which helps prevent test contamination between test runs. The implementation correctly creates a Title object and uses the WikiPage class to handle the deletion.
tests/EntryPoints/SearchEntitiesApiTest.php (1)
131-133: Test preparation improved with compatibility check and property setupThe added lines properly handle two important aspects:
- Skipping tests on PHP 8.1+ to avoid compatibility issues with Wikibase
- Creating the necessary test property before attempting to run the search test
These changes improve test stability and address one of the PR objectives related to updating tests for compatibility with newer dependencies.
tests/Presentation/ConfigPageSmokeTest.php (4)
7-7: Improved test data handling with Valid helper classUsing the Valid class to provide test data improves maintainability and consistency across tests.
15-18: Added proper test cleanupThe tearDown method ensures the configuration page is deleted after each test, preventing state leakage between tests. This is a good practice that improves test isolation.
21-21: Using standardized test data instead of hardcoded valuesReplacing hardcoded configuration with a standardized helper method improves consistency and maintainability.
33-33: Updated expectation to match new default valueThe test now expects a null value for startTimePropertyId instead of "P1", reflecting a change in the application's default configuration. This ensures the test properly validates the current expected behavior.
resources/ext.wikibase.export.js (1)
343-345: Added essential null handling.This change adds important validation to check if the
dataparameter or itsentitiesproperty is falsy, returning an empty array in those cases. This adds robustness and prevents potential JavaScript errors when accessing properties of undefined objects.i18n/vi.json (2)
13-13: Updated configuration page reference.Replaced reference to the deprecated
Special:WikibaseExportConfigwithMediaWiki:WikibaseExport, aligning with the removal of the special page mentioned in the PR objectives.
29-29: Updated configuration page reference.Replaced reference to the deprecated
Special:WikibaseExportConfigwithMediaWiki:WikibaseExport, maintaining consistency with the earlier string update.tests/Application/SearchEntities/SearchEntitiesUseCaseTest.php (1)
112-114: Simplified test helper function.Removed conditional logic that previously checked MediaWiki version to determine which stub class to instantiate. This simplification aligns with the minimum MediaWiki version requirement being increased to 1.39, allowing direct usage of the current
StubEntitySearchHelperimplementation.i18n/zh-hant.json (2)
12-12: Updated configuration page reference.Replaced reference to the deprecated
Special:WikibaseExportConfigwithMediaWiki:WikibaseExport, aligning with the removal of the special page mentioned in the PR objectives.
28-28: Updated configuration page reference.Replaced reference to the deprecated
Special:WikibaseExportConfigwithMediaWiki:WikibaseExport, maintaining consistency with the earlier string update.i18n/zh-hans.json (2)
13-13: Updated configuration link correctlyThis change correctly updates the link from the deprecated
Special:WikibaseExportConfigpage toMediaWiki:WikibaseExport, aligning with the PR objective of removing the outdated special page.
29-29: Updated configuration link correctlyThis change correctly updates the link from the deprecated
Special:WikibaseExportConfigpage toMediaWiki:WikibaseExport, ensuring consistent configuration access throughout the application.i18n/he.json (2)
12-12: Updated configuration link correctlyThis change correctly updates the link from the deprecated
Special:WikibaseExportConfigpage toMediaWiki:WikibaseExport, aligning with the PR objective of removing the outdated special page.
28-28: Updated configuration link correctlyThis change correctly updates the link from the deprecated
Special:WikibaseExportConfigpage toMediaWiki:WikibaseExport, ensuring consistent configuration access throughout the application.tests/TestDoubles/StubEntitySearchHelper.php (1)
27-34: Method signature updated to match MediaWiki interfaceThis change correctly updates the
getRankedSearchResultsmethod signature to include the new nullableprofileContextparameter, aligning with recent MediaWiki extension updates. The method parameters are now properly formatted with one parameter per line for better readability.i18n/ar.json (2)
12-12: Updated configuration link correctlyThis change correctly updates the link from the deprecated
Special:WikibaseExportConfigpage toMediaWiki:WikibaseExport, aligning with the PR objective of removing the outdated special page.
28-28: Updated configuration link correctlyThis change correctly updates the link from the deprecated
Special:WikibaseExportConfigpage toMediaWiki:WikibaseExport, ensuring consistent configuration access throughout the application.i18n/nl.json (2)
13-13: Link path updated correctly.The administrator link has been properly updated from the deprecated Special page to the MediaWiki page, matching the changes across other language files.
29-29: Link path updated correctly.The configuration link has been properly updated to reflect the removal of Special:WikibaseExportConfig.
i18n/sl.json (2)
11-11: Link path updated correctly.The administrator link has been properly updated from the deprecated Special page to the MediaWiki page.
27-27: Link path updated correctly.The configuration link has been properly updated to reflect the removal of Special:WikibaseExportConfig.
src/EntryPoints/SpecialWikibaseExport.php (2)
8-8: Added Message class import.Correctly added import for the Message class to support the method signature change.
41-42: Improved return type for getDescription method.The method now returns a Message object instead of a string, which aligns better with MediaWiki standards for internationalization and improves type safety.
src/Application/SearchEntities/SearchEntitiesUseCase.php (4)
34-36: Improved code readability with ternary operator.The conditional has been refactored to use a more concise ternary operator, making the code cleaner.
54-56: Updated method parameters for compatibility.Added the required parameters for the
getRankedSearchResultsmethod to maintain compatibility with recent MediaWiki changes.
66-70: Added null handling for entity ID.Important null check to prevent potential null reference exceptions, improving the robustness of the code.
88-92: Added null handling for entity ID.Properly implemented null check for entity ID in the filtered search result method, consistent with the earlier implementation.
extension.json (4)
5-5: Version Bump Verification
The version has been updated from "1.0.0" to "2.0.0", which clearly reflects a major update in the extension.
20-21: MediaWiki Requirement Update
The MediaWiki dependency is now set to ">= 1.39.0". This update aligns with the PR objective to support newer MediaWiki versions.
69-71: Removal of Deprecated Special Page
The removal of the "SpecialWikibaseExportConfig" entry from the SpecialPages configuration (now only "WikibaseExport" is listed) is consistent with deprecating the outdated configuration interface.
113-115: Update to Extension Message File Reference
The alias file path has been updated from the old "WikibaseExport.i18n.alias.php" format to "WikibaseExport.alias.php", reflecting the file structure change described in the PR summary.i18n/en.json (2)
15-16: Updated Admin Notice Link
The "wikibase-export-intro-admin-notice" string now correctly links to "[[MediaWiki:WikibaseExport|configure this form]]", replacing the outdated "Special:WikibaseExportConfig" reference. This improves clarity for administrators.
32-33: Consistent Configuration Link Update
The "wikibase-export-config-incomplete-link" has been updated to use "[[MediaWiki:WikibaseExport|configure this form]]", ensuring consistency with the removal of the deprecated configuration special page..github/workflows/ci.yml (10)
9-11: Test Job Enhancement with Continue-on-Error
The addition ofcontinue-on-error: ${{ matrix.experimental }}(line 10) allows jobs marked as experimental to continue despite failures. This update supports more flexible testing scenarios.
12-14: Fail-fast Strategy Update
Settingfail-fast: falseensures that all jobs run to completion even if one fails. This change is beneficial for comprehensive CI feedback.
15-33: Matrix Configuration Refinement
The updated matrix now includes newer MediaWiki versions (REL1_39 to REL1_43 and master) with appropriate PHP versions and experimental flags. This improvement not only lifts support for legacy versions but also facilitates testing of the master branch with experimental settings.
49-57: Cache Key Revision for MediaWiki
The cache key for MediaWiki has been updated to use the suffix_v8(line 57). This helps ensure that cached data is properly segregated according to the new configuration settings.
59-64: Cache Key Revision for Composer Cache
Updating the Composer cache key tocomposer-php${{ matrix.php }}_v2(line 63) is a clear step toward improved cache management in light of dependency changes.
100-103: PHPStan Job Renaming for Clarity
Renaming the PHPStan job to "Static Analysis: MW ${{ matrix.mw }}, PHP ${{ matrix.php }}" (line 102) better reflects the task performed.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 100-100: too many blank lines
(4 > 2) (empty-lines)
124-132: Consistent Cache Key in PHPStan Job
The PHPStan job now also uses the amended cache key (mw_${{ matrix.mw }}-php${{ matrix.php }}_v8on line 132), maintaining consistency with the test job configuration.
164-167: Psalm Step Addition
The inclusion of the Psalm step (lines 164–166) with a conditionif: trueensures that static analysis is always run. This enhances the CI process by providing insight into potential issues.
208-210: Cache Key Consistency in Code Style Job
The cache key for the Code Style job has been updated tomw_${{ matrix.mw }}-php${{ matrix.php }}_v8(line 208), aligning it with the test and PHPStan jobs.
236-238: Node Version Upgrade for Linting
Upgrading the Node version to 22 in the linting job (line 237) ensures compatibility with the latest Node features and tooling, in line with other modern updates in this workflow.i18n/pt-br.json (1)
11-11: Updated references from Special page to MediaWiki page correctly.The link has been updated from
[[Special:WikibaseExportConfig|configurar este formulário]]to[[MediaWiki:WikibaseExport|configurar este formulário]]as part of the PR's objective to remove the outdated special page and use the MediaWiki page instead.phpstan.neon (1)
16-17: Added PHPStan bootstrap configuration properly.Adding the AutoLoader.php bootstrapFile enables PHPStan to properly load MediaWiki classes during static analysis, which will improve code analysis accuracy and fix CI issues.
WikibaseExport.alias.php (2)
2-7: Added proper documentation block.The documentation block improves code clarity by stating the file's purpose and categorization within the Extensions system.
11-14: Added English special page alias correctly.This properly defines the alias for the 'WikibaseExport' special page in English, ensuring the page can be accessed consistently. This is in line with MediaWiki conventions for special page registration.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
164-167: New Psalm Analysis Step
A Psalm step has been added:
- It runs the command
cd extensions/WikibaseExport && php vendor/bin/psalm --shepherd --stats- It includes an
if: truecondition, which is redundant.
Consider removing theif: trueto simplify the configuration. For example, remove the condition so the step simply always runs.- - name: Psalm - run: cd extensions/WikibaseExport && php vendor/bin/psalm --shepherd --stats - if: true + - name: Psalm + run: cd extensions/WikibaseExport && php vendor/bin/psalm --shepherd --stats
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
-
.github/workflows/ci.yml(9 hunks) -
WikibaseExport.alias.php(1 hunks) -
extension.json(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- extension.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Static Analysis: MW REL1_43, PHP 8.3
🔇 Additional comments (11)
WikibaseExport.alias.php (1)
1-15: Well-structured special page aliases implementation.The file properly defines the special page aliases for the WikibaseExport extension following MediaWiki conventions. The documentation and structure are clear, with appropriate array initialization and English language aliases for both WikibaseExport and WikibaseExportConfig special pages.
.github/workflows/ci.yml (10)
9-10: Dynamic Test Failure Handling
The test job now usescontinue-on-error: ${{ matrix.experimental }}so that failures are only ignored when running an experimental configuration. Please verify that this behavior is intentional and that no critical failures will be inadvertently overlooked in experimental runs.
12-33: Expanded Matrix Configuration
The matrix now includes MediaWiki versions from REL1_39 to REL1_43 plus the master branch, with corresponding PHP versions and anexperimentalflag. This improves compatibility testing with newer MediaWiki versions. Ensure that the chosen PHP versions (e.g., 8.0 for REL1_39, 8.1 for REL1_40, etc.) align with the official support for each MediaWiki version.
53-57: Updated Cache Key for MediaWiki
The caching key for the MediaWiki directory has been updated to include the_v8suffix (key: mw_${{ matrix.mw }}-php${{ matrix.php }}_v8). This update should help invalidate stale caches following the CI changes. Please confirm that this update is in line with your caching strategy across all jobs.
59-63: Composer Cache Key Revision
The Composer cache key has been modified tocomposer-php${{ matrix.php }}_v2, ensuring compatibility with the new cache configuration. This improvement should help avoid conflicts with previous cache versions.
78-80: Composer Allow-Plugins Step Added
A new step now configures Composer to allow plugins (composer config --no-plugins allow-plugins.composer/installers true). Make sure this configuration is safe in your CI environment and does not inadvertently permit untrusted plugins.
101-103: Job Name Update for Static Analysis
The PHPStan job has been renamed to "Static Analysis: MW ${{ matrix.mw }}, PHP ${{ matrix.php }}", which is clearer given that multiple static analysis tools are being run.
105-109: PHPStan Matrix Scope Restriction
The static analysis job (PHPStan) now runs only for MediaWiki REL1_43 with PHP 8.3. Please verify that limiting PHPStan analysis to this configuration is sufficient to catch issues across your codebase. If compatibility with other versions is important for static analysis, consider broadening the matrix.
124-132: Consistent Cache Key in Static Analysis Job
In the PHPStan job, the cache key for MediaWiki has also been updated to use the_v8suffix, ensuring consistency with the test job’s caching mechanism.
204-209: Updated Cache Key for Composer in Code Style Job
Within the code-style job, the Composer cache key is updated on line 208 tomw_${{ matrix.mw }}-php${{ matrix.php }}_v8. This aligns the cache key with the updated scheme in other jobs.
237-238: Node Version Upgrade in Linting Job
The node version for the linting job has been updated from 16 to 22. Confirm that all Node dependencies (installed via npm) are compatible with Node 22.
| @@ -112,7 +112,7 @@ | |||
| }, | |||
|
|
|||
| "ExtensionMessagesFiles": { | |||
| "WikibaseExportAlias": "WikibaseExport.i18n.alias.php" | |||
There was a problem hiding this comment.
Doesn't matter much, but I think the old name is clearer (esp for people less familiar with MW), and probably purposefully deviated from the MW standard
| @@ -128,6 +128,9 @@ public function testEdgeToEdge(): void { | |||
| } | |||
|
|
|||
| public function testNoMatchesReturnsEmptyResult(): void { | |||
| $this->skipOnPhp81AndLater(); | |||
There was a problem hiding this comment.
The test is falling for 8.1 and later and I saw similar patterns in other tests too. Do you want me to follow up on the test?
There was a problem hiding this comment.
ProfessionalWiki\WikibaseExport\Tests\EntryPoints\SearchEntitiesApiTest::testNoMatchesReturnsEmptyResult
Wikibase\Repo\Api\EntitySearchException:
/var/www/html/extensions/WikibaseCirrusSearch/src/EntitySearchElastic.php:315
/var/www/html/extensions/Wikibase/repo/includes/Api/CombinedEntitySearchHelper.php:43
/var/www/html/extensions/Wikibase/repo/includes/Api/TypeDispatchingEntitySearchHelper.php:47
/var/www/html/extensions/WikibaseExport/src/Application/SearchEntities/SearchEntitiesUseCase.php:49
/var/www/html/extensions/WikibaseExport/src/Application/SearchEntities/SearchEntitiesUseCase.php:32
/var/www/html/extensions/WikibaseExport/src/EntryPoints/SearchEntitiesApi.php:23
/var/www/html/includes/Rest/SimpleHandler.php:40
/var/www/html/tests/phpunit/unit/includes/Rest/Handler/HandlerTestTrait.php:233
/var/www/html/extensions/WikibaseExport/tests/EntryPoints/SearchEntitiesApiTest.php:134
There was a problem hiding this comment.
/var/www/html/extensions/WikibaseCirrusSearch/src/EntitySearchElastic.php:315
This is caused by your local setup. WikibaseCirrusSearch is not a dependency of this extension.
(This might imply a compatibility issue with WikibaseCirrusSearch, but that's out of scope for now.)
| @@ -23,7 +30,7 @@ public function testEditSmoke(): void { | |||
|
|
|||
| // Default value | |||
| $this->assertStringContainsString( | |||
| '"startTimePropertyId": "P1"', | |||
| '"startTimePropertyId": null', | |||
There was a problem hiding this comment.
This must be some of @malberts his oldest MW code :)
Why didn't you make it a variable in the message so that translations can't get it wrong in the future? |
I didn't realize that it can be done, will create a follow up patch for it |
Key changes:
getOptionsFromEntityData()inext.wikibase.export.jsSpecial:WikibaseExportConfiglink in i18n directly toMediaWiki:WikibaseExportvimeo/psalmfrom^4.22.0to^4.30.0Summary by CodeRabbit
New Features
Bug Fixes