Skip to content

[Store] Add ManagedStoreInterface #328

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 20, 2025

Conversation

Guikingone
Copy link
Contributor

@Guikingone Guikingone commented Aug 19, 2025

Q A
Bug fix? no
New feature? yes
Docs? yes
Issues #285 #314
License MIT

Hi 👋🏻

This PR aim to introduce the ManagedStoreInterface as discussed in #285, Store is a critical part of this library so I'm not gonna pretend that I know how each store currently work but here's the refactoring for most of them, tests must be improved but here's the first "draft".

This PR does not include the related commands.

@carsonbot carsonbot added Feature New feature Store Issues & PRs about the AI Store component Status: Needs Review labels Aug 19, 2025
@OskarStark OskarStark changed the title [Store] Adding ManagedStoreInterface [Store] Add ManagedStoreInterface Aug 19, 2025
Copy link
Contributor

@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

No docs to adjust or changelog?

@Guikingone
Copy link
Contributor Author

Guikingone commented Aug 19, 2025

Yes, need to work on the changelog and tests, won't update the changelog until the tests are fixed 🙂

Regarding the documentation, the current state is that InitializableStoreInterface is the interface for initialization, should we list the change to ManagedStoreInterface or just change the name in the file? 🤔

@OskarStark
Copy link
Contributor

Just change the class name in the changelog as we haven't released yet

@OskarStark
Copy link
Contributor

Can you create an issue for creating the commands? Thanks

@chr-hertel
Copy link
Member

chr-hertel commented Aug 19, 2025

oh wow, nice one 💪

needs a rebase tho after #288

@Guikingone Guikingone force-pushed the store/managed_refactoring branch from c2a17e4 to 7abbb1f Compare August 20, 2025 07:45
@Guikingone Guikingone requested a review from OskarStark August 20, 2025 07:50
Copy link
Member

@chr-hertel chr-hertel left a comment

Choose a reason for hiding this comment

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

Thanks for that @Guikingone!

Didn't test it - and honestly wouldn't know how at this point. The PHPUnit tests won't be sufficient going forward.
For the other integrations we currently use the examples and the runner for doing integration tests - when we bring in the management commands we should also think about some integration test layer here with all the store bridges.

@chr-hertel chr-hertel force-pushed the store/managed_refactoring branch from 7abbb1f to 35aec00 Compare August 20, 2025 09:07
@chr-hertel
Copy link
Member

Thank you @Guikingone.

@chr-hertel chr-hertel merged commit 631c339 into symfony:main Aug 20, 2025
7 checks passed
OskarStark added a commit that referenced this pull request Aug 20, 2025
…Stark)

This PR was merged into the main branch.

Discussion
----------

[Store] Remove obsolete `DroppableStoreInterface`

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| Docs?         | no
| Issues        | Follows #328
| License       | MIT

Commits
-------

051a609 [Store] Remove obsoloete `DroppableStoreInterface`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature Status: Reviewed Store Issues & PRs about the AI Store component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants