-
-
Notifications
You must be signed in to change notification settings - Fork 66
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
Allow using type names in place of class names #118
Conversation
@dunglas Please have a look |
bump @dunglas What do you think of this approach? |
I personally often change class namespaces to better organize code, leading to thorny migrations, so that change would be very welcome! I +1 👍 |
src/Bundle/DependencyInjection/DunglasDoctrineJsonOdmExtension.php
Outdated
Show resolved
Hide resolved
@dunglas Addressed change reqs, added tests and notes in readme. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! I left some minor comments but overall it looks good to me.
src/Bundle/DependencyInjection/DunglasDoctrineJsonOdmExtension.php
Outdated
Show resolved
Hide resolved
@dunglas Also fixed some unrelated issues to make CI green |
@dunglas any chance to see this merge soon ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the long delay. The overall design looks good to me. Here are some last few comments. Thanks for working on this!!
src/Bundle/DependencyInjection/DunglasDoctrineJsonOdmExtension.php
Outdated
Show resolved
Hide resolved
@TamasSzigeti Do you want to make the modification proposed by @dunglas ? Let me know 🙂 |
Thanks for the review, Kévin! @amenophis Thanks for the offer, go ahead if you wish, otherwise I can take care of it this weekend. |
Hi @TamasSzigeti, Seems you did not had time to push update. Thanks |
@dunglas Comments addressed @amenophis Thanks, just did it (: |
Really nice feature 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@dunglas Comments addressed plus I fixed the workflow again:
Might be worth dropping unsupported php and symfony versions in another PR |
Thank you very much for your hard work @TamasSzigeti!! |
Introduce bundle config to map keys to class names
See #63
Allow custom type aliases from bundle config or custom implementation.
Fix unrelated errors to make CI green:
class_exists(BackedEnum::class)
always returns false