Skip to content

Conversation

@alongosz
Copy link
Member

@alongosz alongosz commented Jun 13, 2024

🎫 Issue IBX-8400 on the way to IBX-3183

Related PRs:

Description:

TL;DR;

Merged Core Bundle and Core Repository layer RepositoryFactory classes into one. Deprecated the Bundle one in favor of the Repository one.

Background

We have ambiguous and redundant RepositoryFactory DI definitions and implementations on Bundle and Repository levels respectively.

 Ibexa\Bundle\Core\ApiLoader\RepositoryFactory:
        class: Ibexa\Core\Base\Container\ApiLoader\RepositoryFactory
        arguments:
            - Ibexa\Core\Repository\Repository
            - '%ibexa.api.role.policy_map%'
            - '@Ibexa\Contracts\Core\Repository\LanguageResolver'
        calls:
            - [setContainer, ["@service_container"]]

    Ibexa\Core\Repository\Repository:
        factory: ['@Ibexa\Bundle\Core\ApiLoader\RepositoryFactory', buildRepository]

Basically Ibexa\Bundle\Core\ApiLoader\RepositoryFactory for Repository layer is a named service with Ibexa\Core\Base\Container\ApiLoader\RepositoryFactory as its implementation. Then on Bundle level it's overridden by the actual class implementation.

This causes quite a headache for both rector and static analysis. Moreover to modify any of the Repository services, changes to both classes are needed, which is difficult due to PHPStorm not understanding which definition is being modified. So this also constitutes a maintenance hell. We've also had issues in the past where due to existence of multiple RepositoryFactory definitions (maybe not ATM as it's overridden) wrong instances of services were injected (e.g. uninitialized permission resolver).

I've decided to extract some common abstraction and using it merge Bundle logic with Repository logic and use in DI only the Core Repository RepositoryFactory. ATM the Bundle one is just deprecated, however as in case of #383, I hope to back-port that to 4.6 and drop it here.

Additionally I've dropped support for dynamic Core Repository class passed as RepositoryFactory service argument. This is also difficult to maintain and makes no sense (substitution principle-wise) as it requires for a constructor to have the same argument list. If a custom Repository class is needed, the entire RepositoryFactory should be properly decorated.

All the changes allowed me to drop Container-awareness from RepositoryFactory and use proper dependency injection for RepositoryConfigurationProvider (see #383).

The legacy integration test setup configuration has been modified to have the same effect after changes to the factory.

I've also added an array shape TPolicyMap for policy configuration injected into RoleService through Repository through RepositoryFactory

Changelog:
  • Dropped support for dynamic Core Repository class in RepositoryFactory
  • Made RepositoryFactory not Container-aware
  • Deprecated Core Bundle RepositoryFactory in favor of Repository's one
  • [PHPStan] Aligned baseline with the changes
  • Combined Bundle and core RepositoryFactory into one
  • [Tests] Added needed RepositoryFactory configuration to test setup
  • [PHPStan] Introduced role policy map array shape as TPolicyMap
  • [PHPStan] Aligned baseline with the changes

For QA:

Regressions done via ibexa/commerce#877 should be enough.

Documentation:

  • Deprecated Core Bundle RepositoryFactory in favor of the Core Repository one
  • Dropped support for dynamic Core Repository class FQCN RepositoryFactory argument

@Steveb-p Steveb-p requested a review from a team June 13, 2024 23:59
@konradoboza konradoboza requested a review from a team June 14, 2024 06:20
@alongosz alongosz force-pushed the ibx-8399-move-repository-configuration-provider branch from 16ae74a to 9130e95 Compare June 17, 2024 09:27
Base automatically changed from ibx-8399-move-repository-configuration-provider to main June 18, 2024 12:41
@alongosz alongosz force-pushed the ibx-8400-fix-redundant-repository-factory branch from e3e913f to 139b872 Compare June 18, 2024 12:52
@sonarqubecloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@alongosz
Copy link
Member Author

Note: merging as-is. Regression tests here should be enough.

@alongosz alongosz merged commit f8ff4f6 into main Jun 18, 2024
@alongosz alongosz deleted the ibx-8400-fix-redundant-repository-factory branch June 18, 2024 14:27
barw4 pushed a commit that referenced this pull request Oct 17, 2024
For more details see https://issues.ibexa.co/browse/IBX-8400 and #384

Key changes:

* Dropped support for dynamic Core Repository class in RepositoryFactory

* Made RepositoryFactory not Container-aware

* Deprecated Core Bundle RepositoryFactory in favor of Repository's one

* Combined Bundle and core RepositoryFactory into one

* [Tests] Added needed RepositoryFactory configuration to test setup

* [PHPStan] Introduced role policy map array shape as TPolicyMap

* [PHPStan] Aligned baseline after the changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants