Skip to content

Conversation

@alongosz
Copy link
Member

@alongosz alongosz commented Jun 13, 2024

🎫 Issue IBX-8399 on the way to IBX-8400 and IBX-8138

Related PRs:

Description:

TL:DR;

Effectively moved RepositoryConfigurationProvider from Bundle to Repository layer (for now actually deprecated the old class, but aiming to back-port deprecation to 4.6 and remove it in 5.0 as a follow-up).

Background

We need to refactor DI configuration defining RepositoryFactory (see related PR) so we can fix long outstanding ambiguity issues listed in the JIRA ticket. For now it will help with Symfony 5 Rectors refactoring. The changes done to ./src/bundle/Core/ApiLoader/RepositoryFactory.php are related to that refactoring (RepositoryConfigurationProvider injection instead of $this->container->get() call) and probably can be applied in a simpler manner to resolve the target issue IBX-8138, however I feel like it's the highest time to do that cleanup - it's a low hanging fruit.

But before we can do that, we need to move RepositoryConfigurationProvider from the Bundle to the Repository layer as it's the most inner layer. I've done that in this PR and while at it I've introduced RepositoryConfigurationProviderInterface contracts.

I've deprecated Core Bundle RepositoryConfigurationProvider in favor of the Core Repository layer one (\Ibexa\Core\Base\Container\ApiLoader\RepositoryConfigurationProvider). Ideally I'd like to back-port that deprecation to 4.6, but as a follow-up due to time constraints.

Additionally I've made Ibexa\Bundle\Core\ApiLoader\Exception\* Exceptions final and implementing Repository InvalidArgumentException instead of the core PHP one.

I've also cleaned up some minor parts of code touched around repository configuration provider injection refactoring.

Changelog:
  • Changed Core Bundle ApiLoader Exception to be Repository Exceptions
  • Extracted an interface from RepositoryConfigurationProvider
  • Fixed strict type of CleanupVersionsCommand::$connection property
  • [Tests] Aligned tests with RepositoryConfigurationProvider changes
  • [PHPStan] Aligned baseline with the changes

For QA:

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

Documentation:

  • Deprecation of Core Bundle RepositoryConfigurationProvider in favor of the Core Repository layer one.
  • Core Bundle ApiLoader Exceptions follow now Repository Exceptions contract

@Steveb-p Steveb-p requested a review from a team June 13, 2024 23:56
Copy link
Contributor

@konradoboza konradoboza left a comment

Choose a reason for hiding this comment

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

Apart from below comments and Paweł's remark related to constructor property promotion.

@konradoboza konradoboza requested a review from a team June 14, 2024 06:25
Copy link
Contributor

@webhdx webhdx left a comment

Choose a reason for hiding this comment

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

+1 on other comments, especially on getting rid of ContainerInterface::get calls.

@webhdx webhdx requested a review from a team June 14, 2024 08:28
@alongosz alongosz requested a review from Steveb-p June 14, 2024 16:38
Copy link
Contributor

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

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

LGTM.

Any suggestions from me at this point are merely suggestions, that could also be treated as out of scope :)

@alongosz alongosz force-pushed the ibx-8399-move-repository-configuration-provider branch from 16ae74a to 9130e95 Compare June 17, 2024 09:27
@alongosz alongosz requested review from a team and Steveb-p June 17, 2024 09:58
@sonarqubecloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

@alongosz alongosz merged commit 5daf2a1 into main Jun 18, 2024
@alongosz alongosz deleted the ibx-8399-move-repository-configuration-provider branch June 18, 2024 12:41
@alongosz
Copy link
Member Author

Note: regression tests for now are enough. There are no extra errors there compared to 5.0.x-dev baseline.

barw4 pushed a commit that referenced this pull request Oct 17, 2024
…383)

For more details see https://issues.ibexa.co/browse/IBX-8399 and #383

Key changes:

* Changed Core Bundle ApiLoader Exception to be Repository Exceptions

* Extracted an interface from RepositoryConfigurationProvider

* Refactored StorageConnectionFactory to rely on ServiceProviderInterface

* Added missing readonly keyword in relevant places

* Deprecated RepositoryConfigurationProvider located in Core Bundle namespace

* [Tests] Aligned tests with the changes

* [PHPStan] Aligned baseline with the changes

* [PHPStan] Updated baseline after PHPStan release

---------

Co-Authored-By: Paweł Niedzielski <[email protected]>
Co-Authored-By: Konrad Oboza <[email protected]>
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.

6 participants