refactor: Centralize container management in Config class#5
Merged
Conversation
Move container management from Provider to Config class for better separation of concerns. Add Safe_Dynamic_Prefix utility to handle table name length constraints dynamically. Update all references to use Config::get_container() instead of Provider::get_container(). Key changes: - Container now managed by Config class - Added Safe_Dynamic_Prefix utility for dynamic table prefix calculation - Fixed table name length constraints to respect MySQL's 64-character limit - Updated tests to use new container access pattern 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…to fix/table-prefixes
dpanta94
requested changes
Jul 4, 2025
dpanta94
left a comment
dpanta94
left a comment
Member
There was a problem hiding this comment.
I like it! Left some comments
- Remove schema slug modification from Table_Abstract constructor - Fix container property type hint in Config class - Remove redundant get_container() method from Provider - Use imported RuntimeException instead of fully qualified name 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
dpanta94
approved these changes
Jul 4, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Move container management from Provider to Config class for better separation of concerns. Add
Safe_Dynamic_Prefixutility to handle table name length constraints dynamically. Update all references to useConfig::get_container()instead ofProvider::get_container().Key changes:
Safe_Dynamic_Prefixutility for dynamic table prefix calculationIf I recall correctly, during our review @lucatume may have suggested moving the container from
ConfigtoProvider. If that memory is correct, it didn't register fully in my head. Either way, theConfigapproach is what we do in our other Stellar libraries and we should stick with that.The additional changes tweak the way the prefixes are handling to be a bit more dynamic and separate dynamic table name handling to a single class.