Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes project-creation QA so ICU MessageFormat validation is applied to locked (101% ICE) segments when ICU is enabled for the project, aligning behavior with existing tag-validation flows. It also enables stricter PHPStan @throws checking and regenerates the baseline, adding a backlog file for incremental cleanup.
Changes:
- Pass an
icuEnabledflag intoQAProcessorand create/pass aMessagePatternComparator+sourceContainsIcuintoQAduring project creation. - Add unit tests covering ICU enabled/disabled detection and an end-to-end “broken ICU target produces warnings” scenario.
- Enable new PHPStan exception/throws rules and baseline existing violations (plus a tracking backlog file).
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/Model/ProjectCreation/QAProcessor.php | Adds ICU detection and passes comparator/flag into QA during processing. |
| lib/Model/ProjectCreation/ProjectManager.php | Wires icuEnabled from ProjectStructure::$metadata into QAProcessor. |
| tests/unit/Model/ProjectCreation/QAProcessorTest.php | Adds ICU-focused unit tests (detection and error reporting). |
| tests/unit/Model/ProjectCreation/TestableQAProcessor.php | Updates overridden createQA() signature and captures comparator/flag for assertions. |
| phpstan.neon | Enables stricter throws/exception checking configuration. |
| phpstan-baseline.neon | Regenerated baseline to account for newly enabled PHPStan rules. |
| phpstan-throws-backlog.txt | Adds a list of files to fix sequentially for throws-rule cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 12 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Agent-Logs-Url: https://github.com/matecat/MateCat/sessions/fd68bba9-e500-4dc8-acc5-ca3dc0ec9264 Co-authored-by: Ostico <[email protected]>
Ostico
force-pushed
the
fix-icu-check-not-performed-on-blocked-segments
branch
from
564c188 to
62cfbbc
Compare
12 tasks
…performed-on-blocked-segments
… GetSegmentsController Centralize the last remaining duplicated `containsComplexSyntax() && isValidSyntax()` pattern to use ICUSourceSegmentDetector::sourceContainsIcu(), consistent with all other ICU detection sites (QAProcessor, trait consumers).
Comment on lines
+20
to
+23
| * When ICU is enabled for the project, the processor also detects ICU MessageFormat | ||
| * patterns in each segment and passes a {@see MessagePatternComparator} to QA so that | ||
| * ICU consistency is validated alongside tag checks. | ||
| * |
There was a problem hiding this comment.
The PR description lists additional changes/files (PR readiness workflow + scripts, PR template, phpstan @throws rules, phpstan-throws-backlog.txt) that don’t appear to be present in this PR’s code changes. Please either include those files/changes in the PR, or update the description to match what’s actually being merged to avoid confusion for reviewers and release notes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ICU string validation was silently skipped on locked (101% ICE) segments during project creation.
QAProcessor::createQA()instantiatedQAwithout passingMessagePatternComparatoror the$string_contains_icuflag, causingICUChecker::hasIcuPatterns()to always returnfalse.This PR fixes the bug and consolidates all ICU source-segment detection into a single shared utility (
ICUSourceSegmentDetector), eliminating the last inlinecontainsComplexSyntax() && isValidSyntax()duplication inGetSegmentsController.Type
feat— new user-facing featurefix— bug fixrefactor— restructure without behavior changechore— build, deps, config, docsperf— performance improvementtest— test coverageChanges
lib/Model/ProjectCreation/QAProcessor.php$icuEnabledparam, useICUSourceSegmentDetector::sourceContainsIcu(), pass comparator + flag toQAlib/Model/ProjectCreation/ProjectManager.phpicuEnabledfrom project metadata toQAProcessorlib/Utils/LQA/ICUSourceSegmentDetector.phplib/Utils/LQA/ICUSourceSegmentChecker.phpICUSourceSegmentDetectorlib/Controller/API/App/GetSegmentsController.phpICUSourceSegmentDetector::sourceContainsIcu()tests/unit/Model/ProjectCreation/QAProcessorTest.phptests/unit/Model/ProjectCreation/TestableQAProcessor.phpTesting
vendor/bin/phpunit --exclude-group=ExternalServices --no-coveragepasses./vendor/bin/phpstan analysepasses (2 pre-existing errors inView/APIDoc.php)2118 tests, 17699 assertions — all green.
AI Disclosure
Claude Code (claude-opus-4-6) — ICU fix, shared detector extraction, codebase-wide refactoring audit, test generation.
Notes
Codebase audit confirmed all ICU detection sites now use the shared utility:
ICUSourceSegmentDetector::sourceContainsIcu()directlyICUSourceSegmentDetector::sourceContainsIcu()directly (this PR)ICUSourceSegmentCheckertrait, which delegates to the detector