REL1_43 REL1_44 compatibility#78
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors CI workflow and adds local CI tooling; introduces MW 1.43-specific parser tests and test harness updates; bumps extension to 4.0.0 and raises MediaWiki requirement to >=1.43.0; standardizes MediaWiki FQCN imports across source/tests; updates docs and i18n (adds Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (4)
test-ci.sh (2)
27-32: Hardcoded MediaWiki and PHP versions in the "all" command lack consistency.The
allcommand hardcodesREL1_43and8.3instead of using the script parameters, limiting flexibility and creating divergence from thetestcommand behavior.all) - echo "Running all jobs with REL1_43..." + echo "Running all jobs with $MW_VERSION..." act -j static-analysis --verbose act -j code-style --verbose - act -j test --matrix mw:REL1_43 --matrix php:8.3 --matrix experimental:false --verbose + act -j test --matrix mw:"$MW_VERSION" --matrix php:"$PHP_VERSION" --matrix experimental:false --verbose
1-5: Add a guard clause to verifyactis installed.The script will fail with an unclear error if
actis not available. Consider adding a dependency check.#!/bin/bash # Helper script to test CI workflow locally with act # Usage: ./test-ci.sh [job-name] [mw-version] [php-version] set -e # Verify act is installed if ! command -v act &> /dev/null; then echo "Error: 'act' is not installed. See: https://github.com/nektos/act" exit 1 fi.gitignore (1)
5-9: Add inline comment to clarify local testing artifacts.Consider adding a brief comment above the local testing section to document why these patterns are ignored.
vendor/ + # Local CI/testing artifacts (act, generated directories, logs) .actrc *.log mediawiki/ EarlyCopy/.github/workflows/ci.yml (1)
126-128: Document why parser tests are limited to REL1_43 and master.Parser tests are conditionally executed only on
REL1_43 || master. This may be intentional to save CI resources, but the rationale should be documented.Add an inline comment:
- name: Run parser tests (explicit targets) if: matrix.mw == 'REL1_43' || matrix.mw == 'master' + # Parser tests are resource-intensive; only run on latest stable and development branches run: php tests/parser/parserTests.php --file=extensions/ExternalContent/tests/parser/parserTests.txt
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci.yml(9 hunks).gitignore(1 hunks)test-ci.sh(1 hunks)
🔇 Additional comments (5)
.github/workflows/ci.yml (5)
7-9: Concurrency configuration is correct.The workflow properly cancels in-progress runs and groups by ref/head_ref to prevent duplicate execution.
50-50: PHP extensions are correctly and consistently configured across jobs.The additions of
pdo_sqliteandphp-astare appropriate for MediaWiki compatibility and psalm analysis.Also applies to: 150-150, 220-220
75-94: Checkout and installation steps are well-structured and clearly named.The explicit step naming, conditional MediaWiki installation, and working-directory overrides correctly handle the multi-stage checkout and setup process.
130-199: Static-analysis job is well-structured with clear step naming.The separation of PHPStan and Psalm steps with explicit names improves readability and debugging.
200-265: Code-style job structure is appropriate.The job properly isolates code-style checking and uses the correct tools. However, the cache key collision issue flagged earlier also affects this job.
| @@ -130,23 +159,29 @@ jobs: | |||
| !mediawiki/extensions/ | |||
| !mediawiki/vendor/ | |||
| key: mw_static_analysis | |||
There was a problem hiding this comment.
Cache key collision: static-analysis and code-style share the same MediaWiki cache.
Both jobs use key: mw_static_analysis, causing them to share a single cached MediaWiki installation. If their dependencies differ or if one job modifies the cache, it could corrupt the other job's environment.
Assign distinct cache keys:
# In static-analysis job (line 161):
- key: mw_static_analysis
+ key: mw_static_analysis_php${{ matrix.php }}
# In code-style job (line 231):
- key: mw_static_analysis
+ key: mw_code_style_php${{ matrix.php }}Or if these jobs are truly identical and can safely share a cache, document this intention and add a comment explaining why the shared key is acceptable.
Also applies to: 231-231
🤖 Prompt for AI Agents
.github/workflows/ci.yml around line 161 (and also line 231): the
static-analysis and code-style jobs both use the identical cache key
`mw_static_analysis`, causing a cache collision; change each job to use a
distinct, descriptive cache key (for example include the job name or purpose and
optionally the OS/hash) such as `mw_static_analysis-${{ matrix.os
}}-static-analysis` and `mw_static_analysis-${{ matrix.os }}-code-style`, or if
sharing is intentional add a comment above both keys explaining why a shared
cache is safe and keep a single shared key.
There was a problem hiding this comment.
adding a comment to indicate that a shared cache is intentional
There was a problem hiding this comment.
@freephile, thank you for addressing this! Adding a comment to document that the shared cache between static-analysis and code-style is intentional is exactly the right approach. This will help future maintainers understand that both jobs can safely use the same MediaWiki cache since they have identical requirements.
The comment will clarify that sharing key: mw_static_analysis is by design and prevent confusion down the road. 👍
improve caching:
- Cache Composer v2 path (~/.cache/composer)
- Scope cache keys with hashFiles('**/composer.lock') so caches are invalidated when deps change, and use restore-keys.
add junit.xml log output for visibility of test failures
replace curl | bash codecov upload with the official action codecov/codecov-action@v4 for security and reliability.
change working-directory from bare ~ which does not work to ${{ github.workspace }} which works in GitHub Actions and also in local runners like 'act' https://github.com/nektos/act
add name: and id: keys
If you install 'act' on your host, you can use it with Docker to run all GitHub Actions locally *before* you commit to test changes https://github.com/nektos/act
…able for all jobs pdo_sqlite is needed by act for local testing
|
Sorry, I just realized that the prior work I was trying to build on was in the mw44 branch. I'll rebase these workflow changes onto the mw44 branch; make the suggested test enhancements; respond to CodeRabbit suggestions; run full GitHub Actions pipelines locally and in my fork with a force push which will finally provide an updated PR. |
- add `make parser-mw143` target for testing 1.43 - add REL1_44 to matrixes for static analysis and code style - add comment that a shared MW Core cache is ok for static analysis and code style tests. - use `continue-one-error: true` for intentional non-blocking steps - add README.md for describing the tests
a68e829 to
3f7482c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Makefile (1)
1-1: Optionally addparser-mw143to.PHONYThe new
parser-mw143target is fine; consider marking it as phony to avoid any future confusion with a real file of the same name:-.PHONY: ci test cs phpunit phpcs stan psalm parser +.PHONY: ci test cs phpunit phpcs stan psalm parser parser-mw143Also applies to: 22-23
.github/workflows/ci.yml (1)
118-125: Parser test condition relies on lexicographic string comparison, which is fragile.The condition
if: matrix.mw >= 'REL1_43' || matrix.mw == 'master'works correctly for the current matrix values but is vulnerable to future version naming schemes. For example, if a branch likeREL1_100is added, the string comparison would fail ('REL1_100' < 'REL1_43'lexicographically).A more robust approach would be to use an explicit list or a matrix-level boolean flag:
# Option 1: Explicit version list if: matrix.mw == 'REL1_43' || matrix.mw == 'REL1_44' || matrix.mw == 'master' # Option 2: Use a matrix flag (recommended for future flexibility) # In the test job matrix, add a flag like: # - mw: 'REL1_43' # run-parser-tests: true # parser-test-file: 'parserTests-mw143.txt'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.github/workflows/ci.yml(12 hunks).gitignore(1 hunks)Makefile(1 hunks)README.md(2 hunks)extension.json(2 hunks)i18n/ce.json(1 hunks)i18n/pt-br.json(1 hunks)src/Adapters/EmbedPresenter/CategoryUsageTracker.php(1 hunks)src/Adapters/EmbedPresenter/ParserFunctionEmbedPresenter.php(1 hunks)src/Adapters/EmbedResourceLoader/ParserFunctionEmbedResourceLoader.php(1 hunks)src/Domain/ContentRenderer/CodeRenderer.php(2 hunks)src/EntryPoints/BitbucketFunction.php(1 hunks)src/EntryPoints/EmbedFunction.php(1 hunks)src/EntryPoints/MediaWikiHooks.php(1 hunks)test-ci.sh(1 hunks)tests/Integration/EmbedFunctionIntegrationTest.php(2 hunks)tests/Integration/EmbedFunctionSystemTest.php(1 hunks)tests/TestEnvironment.php(1 hunks)tests/parser/README.md(1 hunks)tests/parser/parserTests-mw143.txt(1 hunks)tests/parser/parserTests.txt(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- i18n/ce.json
- tests/parser/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- test-ci.sh
- .gitignore
🧰 Additional context used
🪛 GitHub Actions: CI
tests/TestEnvironment.php
[error] 30-30: PHPUnit test failure: Call to undefined method MediaWiki\Parser\ParserOutput::getText(). This caused 13 test errors and exit code 2. Likely incompatible MediaWiki version or API change; update tests or code to align with current ParserOutput API. Command failed: 'php tests/phpunit/phpunit.php -c extensions/ExternalContent/ --log-junit tests/junit.xml' (exit code 2).
🪛 LanguageTool
tests/parser/parserTests-mw143.txt
[style] ~50-~50: Using many exclamation marks might seem excessive (in this case: 42 exclamation marks for a text that’s 1015 characters long)
Context: ...#embed:https://localhost/whatever.hax}} !! html <div class="cdx-message cdx-messag...
(EN_EXCESSIVE_EXCLAMATION)
🔇 Additional comments (21)
src/EntryPoints/MediaWikiHooks.php (1)
7-11: LGTM! Namespace updates align with MediaWiki standards.The fully-qualified MediaWiki namespace imports are correct and consistent with MediaWiki's namespace refactoring.
src/EntryPoints/BitbucketFunction.php (1)
7-7: LGTM! Parser namespace update is consistent.The fully-qualified Parser import aligns with the project-wide namespace standardization.
tests/Integration/EmbedFunctionIntegrationTest.php (1)
10-10: LGTM! Import and usage updated consistently.The explicit Title import and updated usage at line 58 properly align with the namespace standardization effort.
Also applies to: 58-58
src/Domain/ContentRenderer/CodeRenderer.php (1)
7-7: LGTM! Html namespace update is correct.The fully-qualified Html import aligns with MediaWiki's namespace structure.
src/Adapters/EmbedPresenter/ParserFunctionEmbedPresenter.php (1)
7-7: LGTM! Html namespace standardization is correct.The fully-qualified Html import is consistent with the project-wide namespace updates.
tests/TestEnvironment.php (2)
8-8: LGTM! Title namespace import is correct.The fully-qualified Title import aligns with MediaWiki's namespace structure.
24-31: TheParserOutput::getText()call is valid in MediaWiki 1.43 and requires no changes. The method accepts an optionalarray $options = []parameter. UsinggetRawText()instead would be incorrect, as it returns cacheable HTML with parser markers still present, not the display-ready HTML thatgetText()provides. If test failures are occurring, verify the actual error messages, as they are unlikely to be caused by this API call.src/Adapters/EmbedPresenter/CategoryUsageTracker.php (1)
7-7: LGTM! Parser namespace import is consistent.The fully-qualified Parser import aligns with the project-wide namespace standardization.
src/EntryPoints/EmbedFunction.php (1)
7-7: LGTM! Parser namespace update is correct.The fully-qualified Parser import is consistent with the namespace standardization across the codebase.
src/Adapters/EmbedResourceLoader/ParserFunctionEmbedResourceLoader.php (1)
7-22: NamespacedParserOutputusage is consistent and safeSwitching to
MediaWiki\Parser\ParserOutputin the import and constructor keeps behavior unchanged and matches the newer MediaWiki naming; no issues spotted.tests/Integration/EmbedFunctionSystemTest.php (1)
9-10: Update to namespacedTitle/Usermatches core APIsUsing
MediaWiki\Title\TitleandMediaWiki\User\Userin the tests is consistent with current core APIs and keeps the test behavior identical.Also applies to: 43-71
tests/parser/parserTests.txt (1)
25-53: Class order change in expected HTML is non-breakingOnly the CSS class ordering in the expected error
<div>s changed; content and structure are the same, so this is a safe adjustment to match current output.README.md (1)
100-106: 4.x requirements, install instructions, and release notes are consistentThe documented PHP/MW requirements, Composer constraint
~4.0, and the 4.0.0 release notes all line up with the new extension version and support policy.Also applies to: 113-114, 249-255
extension.json (1)
3-3: Confirm intentional drop of MediaWiki 1.39–1.42 supportBumping to version
4.0.0and requiringMediaWiki >= 1.43.0matches the new README/platform requirements, but it will stop this extension from loading on 1.39–1.42 even if it was previously compatible—please confirm this support change is intentional.Also applies to: 15-19
tests/parser/parserTests-mw143.txt (1)
1-53: New MW 1.43 parser tests mirror existing coverage correctlyThese tests reproduce the key scenarios with the MW 1.43-specific class ordering and configs, so they cleanly separate 1.43 behavior from newer versions.
i18n/pt-br.json (1)
8-18: pt-BR messages correctly cover new error and category keysThe added pt-BR translations align with the existing message keys and tracking categories and are syntactically valid JSON.
.github/workflows/ci.yml (5)
7-9: ✓ Concurrency control is correctly configured.The concurrency group uses
github.head_ref || github.refto deduplicate runs for the same PR/branch while canceling older runs. This is the standard, safe approach.
44-65: ✓ Caching strategy is well-structured with cascading restore-keys.The MediaWiki cache key includes
runner.osandmatrix.phpto avoid collisions across different environments, and the restore-keys properly cascade from specific to general. The Composer cache uses the standard~/.cache/composerpath for Composer v2 and includeshashFiles('**/composer.lock')for dependency tracking.
90-116: ✓ PHPUnit, junit, and coverage uploads are properly configured.The
continue-on-error: trueflag on PHPUnit makes the non-blocking intent explicit, junit.xml is created by the--log-junitflag even on test failure, and coverage upload is correctly gated to the master branch only. The upgrade tocodecov/codecov-action@v4is the secure, recommended approach.
127-199: ✓ Static-analysis and code-style jobs are well-structured.The matrices correctly focus on REL1_43 and REL1_44, omitting the experimental 'master' branch—a reasonable choice for linting jobs. Step names are clear, extensions are consistent with the test job, and the shared cache key is properly documented.
66-80: ✓ Checkout and working-directory configuration is clear and well-organized.The two-step checkout pattern (EarlyCopy at the repo root, then extension into MediaWiki tree) avoids cache busting and is consistent across all jobs. Working directory switches are explicit and correct.
Also applies to: 174-187, 248-261
MediaWiki core locks psalm to ^4.3.0
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
166-173: Minor: Composer cache key naming slightly misleading given shared usage.Both
static-analysisandcode-stylejobs use the identical Composer cache keycomposer_static_analysis-${{ hashFiles('**/composer.lock') }}. The key name suggests it belongs to static-analysis, but it's intentionally shared with code-style. Consider renaming to something more neutral likecomposer_shared-or adding an inline comment explaining the sharing (similar to the MediaWiki cache comments above).Also applies to: 243-250
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(12 hunks)test-ci.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test-ci.sh
🔇 Additional comments (6)
.github/workflows/ci.yml (6)
7-9: Concurrency group properly prevents duplicate workflow runs.The concurrency setup correctly groups runs by workflow name and ref, with
cancel-in-progress: trueto avoid redundant executions.
44-65: Cache strategy is well-structured with proper restore-keys fallback.The MediaWiki and Composer caching uses appropriate fallback chains:
mw_${{ matrix.mw }}-php${{ matrix.php }}-${{ runner.os }}→mw_${{ matrix.mw }}-php${{ matrix.php }}-→mw_${{ matrix.mw }}-, and Composer cache moved to~/.cache/composerwith hash-based key invalidation. This reduces build times across runs.
90-116: JUnit and code coverage artifact uploads are properly implemented.JUnit output is captured and uploaded per matrix entry; code coverage is correctly scoped to master-only runs with conditional codecov-action@v4 upload. The
continue-on-error: trueflags on both steps allow the workflow to continue even if tests fail, which is appropriate for non-blocking steps.
198-202: Psalm skip condition for PHP 8.4 is reasonable and documented.Skipping Psalm on PHP 8.4 due to MediaWiki core's psalm 4.x lock is a pragmatic temporary workaround. The condition and inline comment clarify the rationale. Consider tracking this as a follow-up once MediaWiki core upgrades to psalm 5.x.
160-164: Shared MediaWiki cache between static-analysis and code-style is properly documented.Both jobs use the same
key: mw_static_analysiswith comments explaining the cache is safe to share. Previous review feedback has been incorporated. Maintain these comments in future edits to prevent accidental cache key divergence.Also applies to: 237-241
119-125: Critical: Parser test condition will skipmasterbranch due to lexicographic string comparison.The condition
if: matrix.mw >= 'REL1_43'uses lexicographic (alphabetical) string comparison, not semantic version comparison. In GitHub Actions,'master' >= 'REL1_43'evaluates to false because'm'<'R'alphabetically. This means parser tests will not run for themastermatrix entry.Replace the string comparison with explicit version matching:
- - name: Run parser tests (explicit targets) - if: matrix.mw >= 'REL1_43' || matrix.mw == 'master' + - name: Run parser tests (explicit targets) + if: matrix.mw == 'REL1_43' || matrix.mw == 'REL1_44' || matrix.mw == 'master'Alternatively, if you plan to support only REL1_43 and later:
- if: matrix.mw >= 'REL1_43' || matrix.mw == 'master' + if: matrix.mw != 'REL1_42'Likely an incorrect or invalid review comment.
|
this iteration on the mw44 branch seems good LGTM |
freephile
changed the title
Use runOutputPipeline instead (available since 1.43), as getText is being removed in MW 1.45. Also use namespaced imports for ParserOptions and User.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/TestEnvironment.php (3)
13-17: Reconsider extending TestCase with direct instantiation.The class extends
PHPUnit\Framework\TestCasebut is instantiated viainstance()callingnew self(). PHPUnit's TestCase is designed to be constructed by the test runner with specific parameters. Direct instantiation bypasses PHPUnit's lifecycle management and may cause issues if any inherited TestCase methods are invoked.If TestCase functionality is not required (no assertions, fixtures, etc. are used in this class), consider removing the inheritance to avoid potential issues.
-class TestEnvironment extends TestCase { +class TestEnvironment {
36-36: Remove trailing whitespace.Line 36 contains trailing whitespace on an otherwise empty line.
37-43: Simplify code by removing dead fallback for older MediaWiki versions.The extension requires
MediaWiki >= 1.43.0, which guarantees thatgetContentHolderText()is available. Themethod_exists()check and fallback togetText()are unnecessary. Simplify to:- // getContentHolderText() is available in MediaWiki 1.43+ - if ( method_exists( $parserOutput, 'getContentHolderText' ) ) { - return $parserOutput->runOutputPipeline( $parserOptions ) - ->getContentHolderText(); - } - // MediaWiki 1.44+ removed getText() in favor of getContentHolderText() - return $parserOutput->getText(); + return $parserOutput->runOutputPipeline( $parserOptions ) + ->getContentHolderText();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test-ci.sh(1 hunks)tests/TestEnvironment.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test-ci.sh
🧰 Additional context used
🧬 Code graph analysis (1)
tests/TestEnvironment.php (1)
src/EmbedExtensionFactory.php (1)
getInstance(37-40)
🔇 Additional comments (2)
tests/TestEnvironment.php (2)
8-11: LGTM!The imports correctly use the MediaWiki namespaced classes (
MediaWiki\Parser\ParserOptions,MediaWiki\Title\Title,MediaWiki\User\User) which aligns with the MW 1.43+ compatibility goal.
28-35: Parser setup looks appropriate for test context.Using
User::newSystemUser('TestUser')forParserOptionsand providing a default context page is a reasonable approach for integration tests. The system user avoids dependency on real user accounts.
Use runOutputPipeline instead (available since 1.43), as getText is being removed in MW 1.45. Also use namespaced imports for ParserOptions and User.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #78 +/- ##
============================================
- Coverage 98.25% 92.03% -6.22%
- Complexity 112 138 +26
============================================
Files 20 27 +7
Lines 286 402 +116
============================================
+ Hits 281 370 +89
- Misses 5 32 +27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/Integration/EmbedFunctionIntegrationTest.php (1)
57-69: ParserOptions and output-pipeline handling look correct; consider clarifying intent / reusing helperThe new
ParserOptions+Titleusage and thegetContentHolderTextvsgetTextbranching mirror the compatibility logic intests/TestEnvironment.php::parse, which is good for MW 1.41–1.44 support.Two small, optional improvements:
- Since the return values of
runOutputPipeline(...)->getContentHolderText()/getText()are ignored and you rely only on side effects (category tracking), consider either assigning to a throwaway variable or adding a brief comment that the calls are intentionally unused, to avoid looking like an oversight.- You might also consider extracting this parse/output-pipeline block into a shared helper (similar to
TestEnvironment::parse) to avoid duplicating version-compatibility logic in multiple places.These are purely readability/DRY suggestions; the current behavior is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Integration/EmbedFunctionIntegrationTest.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Integration/EmbedFunctionIntegrationTest.php (1)
tests/TestEnvironment.php (1)
parse(27-44)
🔇 Additional comments (1)
tests/Integration/EmbedFunctionIntegrationTest.php (1)
10-12: New MediaWiki imports are appropriate and consistentThe added
ParserOptions,Title, andUserimports are all used intestUsageIsTrackedand match the MediaWiki namespaces used elsewhere in the tests; no issues here.
|
Finally have all tests passing, including MW master Use runOutputPipeline()->getContentHolderText() instead of getText(); but preserve the latter for backwards compatibility. Similar to Network extension ProfessionalWiki/Network@5c4470f |
The extension requires MediaWiki >= 1.43.0 which guarantees that getDContentHolderText() is available. no CI
TestEnvironment is a utility/helper class that provides methods for tests to use, but it's not itself a test case. It doesn't need to extend TestCase and doing so was incorrect since it's being manually instantiated rather than run by PHPUnit's test runner. no CI
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/Integration/EmbedFunctionIntegrationTest.php (1)
57-64: Discarded return value may warrant a comment.Line 64 executes
runOutputPipeline()->getContentHolderText()but discards the result. If this is intentional (e.g., to trigger side effects that finalize category tracking), consider adding a brief comment to clarify. Alternatively, if the result might be useful for future assertions, capture it in a variable.- $parserOutput->runOutputPipeline( $parserOptions )->getContentHolderText(); + // Run the output pipeline to finalize parser state before checking categories + $parserOutput->runOutputPipeline( $parserOptions )->getContentHolderText();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/Integration/EmbedFunctionIntegrationTest.php(2 hunks)tests/TestEnvironment.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Integration/EmbedFunctionIntegrationTest.php (1)
tests/TestEnvironment.php (1)
parse(26-38)
🔇 Additional comments (3)
tests/Integration/EmbedFunctionIntegrationTest.php (1)
10-12: LGTM on the updated imports.The fully-qualified MediaWiki namespace classes align with MW 1.43+ API conventions and are consistent with changes in
TestEnvironment.php.tests/TestEnvironment.php (2)
8-10: LGTM on the updated imports.The fully-qualified MediaWiki namespace classes properly align with MW 1.43+ API conventions.
26-37: Clean implementation of MW 1.43+ parser API.The refactored
parse()method correctly usesParserOptionswith a system user and routes throughrunOutputPipeline()->getContentHolderText(), which is the modern MediaWiki approach for obtaining processed parser output. ThegetContentHolderText()method is a stable convenience accessor for the rendered content holder, and the pipeline-based pattern aligns with MediaWiki's recommended approach for post-processing parser output.
parser-mw143 is not a physical file
avoid weak string comparison which is vulnerable to failure or ambiguity
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
127-151: Static-analysis job: shared MW/composer caches and Psalm gating are reasonable
- Matrix for
REL1_43/PHP 8.3 andREL1_44/PHP 8.4 gives coverage on the supported branches without over‑expanding permutations.setup-phpmirrors the test job (includingpdo_sqliteandphp-ast), which keeps runtime vs static environments consistent.- MediaWiki cache:
- The shared
mw_static_analysiscache key between static-analysis and code-style is now clearly documented as intentional.- Since both jobs consume the same MW core without mutating it, this shared key is acceptable and saves install time; just keep the comment accurate if the jobs diverge in the future.
- Composer cache:
- Central
composer_shared-${{ hashFiles('**/composer.lock') }}key across the analysis/style jobs is fine, given they rely on the same lockfiles.- EarlyCopy checkout + “Install MediaWiki (if needed)” with
working-directory: ${{ github.workspace }}matches the test job and improves runner/actcompatibility.- Psalm:
- Skipping Psalm on PHP 8.4 because MW locks Psalm to ^4.3 is a pragmatic guardrail.
- This is a good place for a future follow‑up once MediaWiki bumps Psalm (e.g., a TODO or tracking issue), but the current condition is correct.
Overall this job is well‑structured and consistent with the test job; no blocking issues from these changes.
Also applies to: 152-165, 166-173, 174-188, 189-197, 198-203
🧹 Nitpick comments (2)
Makefile (1)
1-1: Newparser-mw143target is consistent and safeThe additional phony target and
parser-mw143rule cleanly wire up the MW 1.43‑specific parser tests and match how CI callsparserTests-mw143.txt. No functional issues here.Static analysis warnings about missing phony
all/cleanare repo‑wide style concerns rather than regressions from this change; only address them if you want to appeasecheckmake(e.g., by adding aggregator / no‑op targets later).Also applies to: 19-23
.github/workflows/ci.yml (1)
7-9: Test job: concurrency, matrix, caching, coverage, and parser tests look coherent
- The
concurrencygroup using${{ github.workflow }}-${{ github.head_ref || github.ref }}is a solid way to cancel redundant runs per ref.- Matrix entries for
REL1_43(PHP 8.3),REL1_44(PHP 8.4), andmaster(PHP 8.4, experimental) align with the stated compatibility goals.- Adding
pdo_sqliteandphp-astinsetup-phpmatches later usage (sqlite in MW, php-ast for static tools) and should be safe in CI.- MediaWiki caching:
- Keyed by MW version, PHP, and runner OS with restore‑keys by MW/PHP is a good balance of isolation and reuse.
- Excluding
mediawiki/extensions/andmediawiki/vendor/ensures core is cached while extension + vendor are refreshed per run.- Composer cache:
- Caching
~/.cache/composerper PHP version +hashFiles('**/composer.lock')will give stable re‑use while still invalidating on lockfile changes.- Split checkout (
EarlyCopyfor scripts and a second checkout intomediawiki/extensions/ExternalContent) is a clean pattern and plays well with the MediaWiki installer script.- Switching
Install MediaWikito run withworking-directory: ${{ github.workspace }}is the right fix for both GitHub‑hosted runners andact.- PHPUnit:
- A first run with JUnit logging and
continue-on-error: trueguarantees test results are captured and artifacts uploaded even on failures.- A second run with coverage gated to
matrix.mw == 'master'keeps the heavier coverage work scoped to master only.- Using Codecov’s v4 action with
continue-on-error: truemeans coverage upload issues won’t break the build, which is appropriate.- Parser tests:
- The explicit REL1_43 branch to use
parserTests-mw143.txtand the default path for REL1_44/master match the new Makefile target and parser test files.- Conditions on
matrix.mwmake the intent clear and match the matrix values.Only optional thought: for master you now run PHPUnit twice (once for JUnit, once for coverage). If CI time ever becomes a concern, you could consolidate into a single run that writes both JUnit and Clover outputs, but what you have is perfectly acceptable.
Also applies to: 16-29, 36-41, 44-56, 57-65, 66-80, 81-89, 90-99, 100-117, 118-125
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(12 hunks)Makefile(2 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
204-227: Code-style job: mirrors static-analysis structure; cache and setup choices look good
- Matrix for code-style matches static-analysis (REL1_43/PHP 8.3 and REL1_44/PHP 8.4), which keeps style checks aligned with supported targets.
- PHP setup and extensions (including
pdo_sqliteandphp-ast) mirror other jobs, avoiding environment drift.- MediaWiki and composer caches are intentionally shared with static-analysis and documented, which is acceptable as long as both jobs continue to use the same MW core and composer setup.
- The EarlyCopy checkout + conditional MediaWiki install from
${{ github.workspace }}keeps this job consistent with the others and compatible with local runners.- Running
composer installinmediawiki/extensions/ExternalContentbeforevendor/bin/phpcsis the right order.The naming tweak to “PHP CodeSniffer” also makes the last step’s purpose clearer. No further changes needed here.
Also applies to: 229-242, 243-250, 251-265, 266-271, 272-273
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Integration/EmbedFunctionIntegrationTest.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Integration/EmbedFunctionIntegrationTest.php (2)
tests/TestEnvironment.php (1)
parse(26-38)src/EmbedExtensionFactory.php (1)
getInstance(37-40)
🔇 Additional comments (2)
tests/Integration/EmbedFunctionIntegrationTest.php (2)
10-12: LGTM! Standardized FQCN imports for MW 1.43+ compatibility.The fully-qualified class name imports for ParserOptions, Title, and User are necessary for the updated parser API usage and align with the PR's goal to standardize MediaWiki imports.
70-74: LGTM! Assertions correctly updated.The assertions now reference
$parserOutputconsistently, which properly reflects the refactored parser invocation pattern.
The runOutputPipeline() call is necessary to populate categories and other metadata, but we need to use the returned object for the assertions.
|
bump |
|
note: because this commit is only on 'master', I still get the 'invalid magic word' error from |
There was a problem hiding this comment.
Pull request overview
This pull request upgrades the ExternalContent extension to support MediaWiki 1.43 and 1.44, marking a major version bump to 4.0.0. The changes primarily focus on namespace modernization, CI/CD improvements, and handling differences in MediaWiki's Codex message rendering between versions.
Changes:
- Updated namespace imports from global to MediaWiki-namespaced classes (Parser, Title, User, Html, ContentHandler, ParserOutput, ParserOptions)
- Enhanced GitHub Actions CI with improved caching, concurrency control, and secure codecov upload
- Added version-specific parser tests to handle different Codex CSS class ordering between MW 1.43 and 1.44+
- Added internationalization updates (Brazilian Portuguese expansion, new Chechen locale)
- Updated minimum MediaWiki version from 1.39 to 1.43
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| extension.json | Bumped version to 4.0.0 and raised minimum MediaWiki requirement to 1.43.0 |
| README.md | Updated platform requirements, installation instructions, and added 4.0.0 release notes |
| .github/workflows/ci.yml | Overhauled CI with concurrency groups, expanded matrices (REL1_43, REL1_44), improved caching, and secure codecov integration |
| src/EntryPoints/*.php | Updated imports to use MediaWiki\Parser\Parser namespace |
| src/Domain/ContentRenderer/CodeRenderer.php | Updated Html import to MediaWiki\Html\Html and removed trailing whitespace |
| src/Adapters/**/*.php | Updated Parser, ParserOutput, and Html imports to use MediaWiki namespaces |
| tests/TestEnvironment.php | Updated to use namespaced classes and new runOutputPipeline() API for MW 1.43+ |
| tests/Integration/*.php | Updated imports to use MediaWiki\Title\Title, MediaWiki\User\User, and MediaWiki\Parser\ParserOptions |
| tests/parser/parserTests.txt | Updated expected HTML to reflect MW 1.44+ Codex class ordering |
| tests/parser/parserTests-mw143.txt | Added MW 1.43-specific parser tests with legacy Codex class ordering |
| tests/parser/README.md | Added documentation explaining version differences and test execution |
| Makefile | Added parser-mw143 target for running MW 1.43 parser tests |
| test-ci.sh | Added helper script for local CI testing with 'act' |
| .gitignore | Added entries for local CI/testing artifacts |
| i18n/pt-br.json | Expanded translations with additional message keys |
| i18n/ce.json | Added new Chechen locale with full translations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| key: mw_static_analysis | ||
| restore-keys: | | ||
| mw_static_analysis |
There was a problem hiding this comment.
The cache key 'mw_static_analysis' is shared across different MediaWiki versions (REL1_43 and REL1_44) in the matrix. This will cause cache conflicts where one version's MediaWiki installation may be used for the other version. The cache key should include the matrix.mw variable to differentiate between versions, for example: 'mw_static_analysis_${{ matrix.mw }}'.
| key: mw_static_analysis | |
| restore-keys: | | |
| mw_static_analysis | |
| key: mw_static_analysis_${{ matrix.mw }} | |
| restore-keys: | | |
| mw_static_analysis_${{ matrix.mw }} |
| files: coverage.xml | ||
|
|
||
| - name: Run parser tests | ||
| run: php tests/parser/parserTests.php --file=extensions/ExternalContent/tests/parser/parserTests.txt | ||
| if: matrix.mw >= 'REL1_43' | ||
| - name: Upload coverage artifact (master only) | ||
| if: matrix.mw == 'master' | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: coverage-${{ matrix.mw }}-php${{ matrix.php }} | ||
| path: coverage.xml |
There was a problem hiding this comment.
The path to the coverage.xml file may be incorrect. The file is written to 'coverage.xml' relative to the 'mediawiki' working directory (line 102), which means it's at 'mediawiki/coverage.xml' from the workspace root. The codecov-action and upload-artifact actions run from the workspace root, so the path should be 'mediawiki/coverage.xml' instead of 'coverage.xml'.
| key: mw_static_analysis | ||
| restore-keys: | | ||
| mw_static_analysis |
There was a problem hiding this comment.
The cache key 'mw_static_analysis' is shared across different MediaWiki versions (REL1_43 and REL1_44) in the matrix. This will cause cache conflicts where one version's MediaWiki installation may be used for the other version. The cache key should include the matrix.mw variable to differentiate between versions, for example: 'mw_static_analysis_${{ matrix.mw }}'.
| key: mw_static_analysis | |
| restore-keys: | | |
| mw_static_analysis | |
| key: mw_static_analysis_${{ matrix.mw }} | |
| restore-keys: | | |
| mw_static_analysis_${{ matrix.mw }} |
|
|
||
| - name: Run PHPUnit | ||
| run: php tests/phpunit/phpunit.php -c extensions/ExternalContent/ | ||
| - name: Run PHPUnit (junit) |
There was a problem hiding this comment.
I am not sure about Junit. We are not consuming it anywhere in the CI pipeline, so it is not needed here.
There was a problem hiding this comment.
I'm not sure how the code coverage pipeline works, but the --log-junit option to PHPUnit produces a JUnit compatible log file which can be ingested by more coverage analysis tools.
| - name: Run PHPUnit | ||
| run: php tests/phpunit/phpunit.php -c extensions/ExternalContent/ | ||
| - name: Run PHPUnit (junit) | ||
| continue-on-error: true |
There was a problem hiding this comment.
We should fail the CI pipeline if PHPUnit doesn't pass. This sliences the error.
There was a problem hiding this comment.
I have to come back to look at this in more detail to understand what I did but I believe the intention to "not fail" was simply to make coverage non-blocking... while test failures are blocking. In any case, project policies and preferences are to be respected.
| - name: Upload code coverage (master only) securely | ||
| if: matrix.mw == 'master' | ||
| continue-on-error: true | ||
| uses: codecov/codecov-action@v4 |
There was a problem hiding this comment.
This needs a CODECOV_TOKEN. @malberts do we have one for this repo?
There was a problem hiding this comment.
Maybe just omit for now? I don't want to deal with this now and getting compat fixed is still nice
| - name: Run parser tests | ||
| run: php tests/parser/parserTests.php --file=extensions/ExternalContent/tests/parser/parserTests.txt | ||
| if: matrix.mw >= 'REL1_43' | ||
| - name: Upload coverage artifact (master only) |
There was a problem hiding this comment.
If we already have Codecov report, should we upload the coverage artifact from the CI?
fix path to junit.xml Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Except for a failing test in REL1_41, this new workflow for GitHub Actions appears to pass all tests for MediaWiki and PHP including REL1_43.
The "incompatible" warning on mediawiki.org could be removed.
The Extension request at Miraheze could be approved
In this fork/branch, all I did was enhance the GitHub Actions pipeline.
I used "act" to run the GitHub Actions pipeline locally in order to test the results.
If you install 'act' on your host, you can use it with Docker to run
all GitHub Actions locally before you commit to test changes
https://github.com/nektos/act
Here are the changes to ci.yml
add concurrency group to avoid duplicate runs for the same ref.
improve caching:
add junit.xml log output for visibility of test failures
replace curl | bash codecov upload with the official action codecov/codecov-action@v4 for security and reliability.
change working-directory from bare ~ which does not work to ${{ github.workspace }} which works in GitHub Actions and also in local runners like 'act' https://github.com/nektos/act
add name: and id: keys
Summary by CodeRabbit