Skip to content

6896: Symfony 8 support, test coverage, CI restructure#27

Merged
turegjorup merged 34 commits intodevelopfrom
feature/symfony-8
Mar 20, 2026
Merged

6896: Symfony 8 support, test coverage, CI restructure#27
turegjorup merged 34 commits intodevelopfrom
feature/symfony-8

Conversation

@turegjorup
Copy link
Copy Markdown
Contributor

@turegjorup turegjorup commented Mar 9, 2026

Ticket

6896

Summary

  • Add Symfony 8 support to all dependency version constraints
  • Fix Symfony 8 breaking changes (Configuration node validation, kernel return types)
  • Increase test coverage from 27% to 100% with new test classes for all untested components
  • Set phpstan to max level and fix all type safety issues in source code
  • Upgrade to PHPUnit 12
  • Update docker compose to itk-version 3.2.4 (networks, COMPOSE_USER, markdownlint/prettier services)
  • Align all GitHub workflows with itkdev-docker templates
    • Split monolithic pr.yaml into changelog.yaml, composer.yaml, php.yaml
    • Add markdown.yaml and yaml.yaml workflows
    • Use COMPOSE_USER: runner env and docker network create frontend steps
    • Use docker compose services (phpfpm, phpfpm84, phpfpm85) instead of shivammathur/setup-php
    • Add composer normalize and composer audit checks
  • Replace linting config with itkdev-docker templates
    • Switch .php-cs-fixer.dist.php to standard @Symfony rules
    • Replace .markdownlint.json with .markdownlint.jsonc and add .markdownlintignore
    • Add .prettierrc.yaml for YAML formatting
    • Remove package.json (markdownlint now via docker)
  • Remove unused dev dependencies (escapestudios/symfony2-coding-standard, kubawerlos/php-cs-fixer-custom-fixers)
  • Apply @Symfony coding standards to PHP files
  • Fix markdown and YAML formatting per linter rules
  • Add Taskfile.yml for local development (setup, lint, analyze, test, test:matrix, pr:actions)
  • Update README development section to document Taskfile workflow
  • Update README "Symfony Native OIDC Support" note: OIDC discovery shipped in 7.3, multiple providers now
    supported natively, clarify this bundle is still needed for authorization code flow (browser login)

Test plan

  • Verify all CI workflows pass (changelog, composer, markdown, yaml, PHP coding standards, PHPStan, unit tests)
  • Verify unit tests pass on PHP 8.3, 8.4, and 8.5 (prefer-lowest and prefer-stable)
  • Verify bundle works with Symfony 6.4, 7.x, and 8.x applications
  • Run task pr:actions locally to validate full CI suite

🤖 Generated with Claude Code

turegjorup and others added 7 commits March 9, 2026 12:49
Add ^8.0 version constraint to all symfony/* packages in require
and require-dev.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add trailing commas to multiline constructor parameters as required
by php-cs-fixer.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove all ignoreErrors and set level to max
- Add PHPDoc array shapes for config parameters
- Add is_string() guards for mixed values from cache and session
- Add UserInterface generic to UserProviderInterface
- Fix return type annotation on validateClaims()

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add new test classes:
- CliLoginTokenAuthenticatorTest
- OpenIdConfigurationProviderManagerTest
- UserLoginCommandTest
- ConfigurationTest
- ItkDevOpenIdConnectExtensionTest
- TestInvalidArgumentException helper

Update existing tests:
- Add empty nonce and missing code tests to OpenIdLoginAuthenticatorTest
- Add cache exception path tests to CliLoginHelperTest
- Add bundle method tests to ItkDevOpenIdConnectBundleTest
- Use createStub() instead of createMock() where no expectations are set

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Update phpunit/phpunit constraint from ^11.0 to ^12.0
- Update phpunit.xml.dist schema to 12.5

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 9, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (de3ac74) to head (7e79e7e).
⚠️ Report is 4 commits behind head on develop.

Additional details and impacted files
@@               Coverage Diff               @@
##             develop       #27       +/-   ##
===============================================
+ Coverage      74.50%   100.00%   +25.49%     
- Complexity        54        56        +2     
===============================================
  Files              9         9               
  Lines            255       248        -7     
===============================================
+ Hits             190       248       +58     
+ Misses            65         0       -65     
Flag Coverage Δ
unittests 100.00% <100.00%> (+25.49%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Split monolithic pr.yaml into separate workflow files following
itk-dev/devops_itkdev-docker template pattern. Use docker compose
services instead of shivammathur/setup-php. Add PHP 8.4 and 8.5
containers for testing. Fix Symfony 8 breaking changes in
Configuration and test kernel.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@turegjorup turegjorup changed the title 6896: Symfony 8 support, test coverage, and tooling upgrades 6896: Symfony 8 support, test coverage, CI restructure Mar 9, 2026
turegjorup and others added 15 commits March 9, 2026 13:46
This is a library without a committed composer.lock, so composer
install fails trying to create one with permission errors inside
the docker container.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Run composer update as the host user to allow writing composer.lock
in the mounted volume. Subsequent commands can run as the default
container user since vendor/ and composer.lock are now writable.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Normalize composer.json version constraints (| to ||). Run all
docker compose commands as host user to avoid file permission
issues. Add XDEBUG_MODE=coverage for unit test coverage. Remove
hardcoded build/ report paths from phpunit.xml.dist.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add prefer-stable and prefer-lowest matrix to unit tests and
composer validate jobs. Rename all job names to match the required
check names from branch protection rules.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use template-style naming: no custom name on changelog job,
consistent format for matrix job names. Update branch protection
required checks to match.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add networks, COMPOSE_USER, extra_hosts, and markdownlint/prettier
tooling services matching the itkdev-docker template.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace .markdownlint.json with .markdownlint.jsonc, add
.markdownlintignore, .prettierrc.yaml, and simplify .php-cs-fixer.dist.php
to standard @symfony rules. Remove package.json (markdownlint now via docker).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add COMPOSE_USER env, docker network creation steps, devops header
comments. Add markdown.yaml and yaml.yaml workflows. Use composer
install instead of update for deterministic CI builds.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove escapestudios/symfony2-coding-standard and
kubawerlos/php-cs-fixer-custom-fixers. Rename composer scripts to
match itkdev conventions.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove redundant {@inheritdoc} tags and superfluous phpdoc param
annotations as required by the standard @symfony php-cs-fixer rules.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add blank lines around headings/lists in CHANGELOG.md, fix long line
and heading level in README.md, normalize YAML quoting via prettier.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Provides setup, lint, analyze, test, and test:matrix tasks matching
the CI workflow. Includes pr:actions to run all checks locally.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace manual docker compose commands with task-based workflow.
Document setup, lint, analyze, test, test:matrix, and pr:actions tasks.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
OIDC discovery shipped in Symfony 7.3 and multiple providers are now
supported natively. Clarify that Symfony's OIDC support covers bearer
token validation only, not the authorization code flow (browser login)
that this bundle provides.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@turegjorup turegjorup self-assigned this Mar 10, 2026
@turegjorup turegjorup requested a review from jekuaitk March 10, 2026 10:08
->children()
->scalarNode('cache_pool')
->info('Method for caching')
->defaultValue('cache.app')
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we remove this default?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Newer version of Symfony does not allow defaults for required values

$loader->load('services.yaml');

$configuration = new Configuration();
/** @var array{cache_options: array{cache_pool: string}, cli_login_options: array{route: string}, user_provider: string|null, openid_providers: array<string, array{options: array<string, mixed>}>} $config */
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not very readable, but i suppose this is generated by our coding standards?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's for phpstan more than us. I'm no fan of the using array's like this, exactly because it can't be type safe except through obscure docblocks. But that's Symfony. They went back ta arrays recently: https://symfony.com/blog/new-in-symfony-7-4-better-php-configuration eventhough some of the community objected symfony/symfony#62092

Comment on lines +17 to +29
* @param array{
* default_providers_options: array<string, mixed>,
* providers: array<string, array{
* metadata_url: string,
* client_id: string,
* client_secret: string,
* redirect_uri?: string,
* redirect_route?: string,
* redirect_route_parameters?: array<string, string>,
* leeway?: int,
* allow_http?: bool
* }>
* } $config
Copy link
Copy Markdown
Collaborator

@jekuaitk jekuaitk Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've never added such in depth parameter docs before. Why now and which tool does it come from?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to get to max level in phpstan, that requires better documentation of array shapes. This matches what is defined in ItkDev\OpenIdConnectBundle\DependencyInjection\Configuration

turegjorup and others added 2 commits March 12, 2026 09:26
- Remove lone @param doc on UserLoginCommand constructor (inconsistent)
- Remove @codeCoverageIgnore and unreachable is_string guard
- Restore cache_pool defaultValue('cache.app') in Configuration
- Remove verbose @param array shape on OpenIdConfigurationProviderManager
- Remove @var type annotation on ItkDevOpenIdConnectExtension
- Revert PHPStan from max to level 8 with targeted ignoreErrors
- Remove overly deep testGetContainerExtension and testGetPath tests

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
turegjorup and others added 6 commits March 16, 2026 13:22
Each PHP version × dependency set combination gets its own vendor
volume, eliminating cross-contamination between lowest/stable runs.
A shared composer-cache volume avoids re-downloading packages.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Derive the service name from PHP version and DEPS variables instead of
a manual mapping. Cache the resolved composer.lock inside each vendor
volume so repeat runs use composer install (no re-resolution). Add
test:matrix:reset to wipe volumes and force a fresh resolve.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove redundant isRequired() on cache_pool (already has defaultValue).
Replace verbose always/then validation with ifTrue/thenInvalid.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Composer validation/audit is not linting — move it out of the lint
aggregate and into pr:actions as a standalone step.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix @var annotation syntax for PHPStan (/* → /**)
- Add phpfpm84 and phpfpm85 docker compose services for CI
- Fix YAML formatting per prettier

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@turegjorup turegjorup requested a review from jekuaitk March 16, 2026 12:52
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
test:run:
desc: "Run tests for a PHP version and dependency set (e.g. task test:run PHP=8.4 DEPS=lowest)"
vars:
PHP: '{{.PHP | default "8.3"}}'
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

task test:run will fail here. The default "8.3" will never kick in as the global PHP definition from line 7 will just be used.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch 👍 Fixed 😄

…rs to prevent them from starting in default dev setup
…HP parameter

The global PHP (docker compose exec command) was shadowing the test:run task's PHP parameter, so task test:run always used the global value and the default "8.3" never kicked in. Renaming to PHP_EXEC fixes task test:run and task test:run PHP=8.4 DEPS=lowest.
@turegjorup turegjorup requested a review from jekuaitk March 20, 2026 10:29
@turegjorup turegjorup merged commit 2656790 into develop Mar 20, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants