Skip to content

Tidy the bundle's structure and constraints - #53

Open
loevgaard wants to merge 1 commit into
fix/27-public-apifrom
fix/30-bundle-structure
Open

Tidy the bundle's structure and constraints#53
loevgaard wants to merge 1 commit into
fix/27-public-apifrom
fix/30-bundle-structure

Conversation

@loevgaard

Copy link
Copy Markdown
Member

Fixes #30

Stacked on #52.

What is in

One source of truth for the tag bag check. Configuration::isTagBagBundleInstalled() and the four Assert calls in the extension both decided what counts as an installed tag bag bundle, with the package name and ^3.0 written twice. Both now call InstalledBundles::hasTagBagBundle(), and the constraint is a constant.

Symfony's exception types. The extension used Webmozart\Assert\Assert::true() and ::keyExists(), which throw \InvalidArgumentException. Symfony's convention for "bundle X requires bundle Y" is \LogicException, so that is what it throws now, with a message that also names the way out (client_side.enabled: false). The integration test asserting on the old type is updated.

Dependency constraints. symfony/event-dispatcher-contracts drops the ^2.5 branch: 2.x pairs with Symfony 5.4, which this bundle no longer supports, and Symfony 6.4's event dispatcher accepts ^3. Added a suggest section for nyholm/psr7, symfony/http-client, setono/tag-bag-bundle and setono/consent-bundle, which the README already told people to install.

What is deliberately out

AbstractBundle and PHP service configuration. The XML DI format is deprecated in Symfony 7.4 and removed in 8.0, so this migration is needed for Symfony 8 support, which is blocked on dependencies rather than on this bundle: setono/bot-detection-bundle allows ^5.4 || ^6.4 || ^7.0, and the consent and tag bag bundles have no Symfony 8 constraints either. Doing the migration now would be churn with no user-visible effect, so the extension keeps its comment marking the spot, and this stays with the Symfony 8 work.

Moving symfony/messenger to suggest. Server side tracking is on by default, so messenger is required for the default configuration. Making it optional would mean guarding the message and handler classes, which costs more than it saves.

@codecov

codecov Bot commented Sep 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (fix/27-public-api@ab2f04b). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...ncyInjection/SetonoMetaConversionsApiExtension.php 84.61% 2 Missing ⚠️
src/DependencyInjection/InstalledBundles.php 75.00% 1 Missing ⚠️
Additional details and impacted files
@@                 Coverage Diff                  @@
##             fix/27-public-api      #53   +/-   ##
====================================================
  Coverage                     ?   81.97%           
  Complexity                   ?      171           
====================================================
  Files                        ?       35           
  Lines                        ?      538           
  Branches                     ?        0           
====================================================
  Hits                         ?      441           
  Misses                       ?       97           
  Partials                     ?        0           

☔ View full report in Codecov by Harness.
📢 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.

Move the duplicated tag bag detection into InstalledBundles, replace the
Webmozart assertions in the extension with LogicException, which is what
Symfony uses for a missing bundle, drop the dead
event-dispatcher-contracts ^2.5 branch and list the optional packages
under suggest.

The AbstractBundle and PHP config migration stays out: it is only needed
for Symfony 8, which is blocked on dependencies.

Fixes #30
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.

1 participant