Skip to content
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

Update doctrine configuration #149

Merged
merged 4 commits into from
Feb 8, 2024
Merged

Update doctrine configuration #149

merged 4 commits into from
Feb 8, 2024

Conversation

JoppeDC
Copy link
Contributor

@JoppeDC JoppeDC commented Feb 2, 2024

This PR References issue #148

  • The doctrine XML config has been moved to attributes
  • The custom migrations have been removed from the bundle. These should be generated in the project itself

Note:
The overwriting of the Sylius resources has also been removed. The test application should probably be updated to reflect the newer way of working

Feedback is appreciated

@mamazu
Copy link
Collaborator

mamazu commented Feb 2, 2024

The PR looks good so far.
One thing that we should probably mention in the docs/upgrade guide.

I am not sure what you mean with the "newer way of working".

@JoppeDC
Copy link
Contributor Author

JoppeDC commented Feb 3, 2024

I have updated the Readme to reflect the changes in regards to migrations. I've also added a small UPGRADE doc, documenting the changes to the migrations and ORM mapping.

Please feel free to let me know if you have any more changes you'd like to see!

@mamazu mamazu marked this pull request as ready for review February 3, 2024 19:52
@mamazu
Copy link
Collaborator

mamazu commented Feb 3, 2024

Perfect. I think this would probably be tagged as 4.0 then as this is a BC break.

I'm not sure if that's going to play well with other projects still using XML (and wanting to use this package). Is this possible? I'll do some testing in our Sylius project to see if it still works.

@JoppeDC
Copy link
Contributor Author

JoppeDC commented Feb 5, 2024

Unfortunatly i do not currently have a project that uses XML mapping, although this should not be a problem.

@mamazu
Copy link
Collaborator

mamazu commented Feb 6, 2024

Anything else that's needs to be done? Otherwise I'd merge it and tag it as the new version.

@JoppeDC
Copy link
Contributor Author

JoppeDC commented Feb 7, 2024

I'm currently using it pinned on my fork branch, and during my testing it has been working fine.

Maybe a rename of the UPGRADE markdown file based on the tag you're planning to push?

@mamazu
Copy link
Collaborator

mamazu commented Feb 7, 2024

Sure, that would be great. It's planned as 4.0

@JoppeDC
Copy link
Contributor Author

JoppeDC commented Feb 7, 2024

Awesome, i've updated the readme @mamazu

@mamazu mamazu merged commit 8983bd2 into Brille24:master Feb 8, 2024
0 of 6 checks passed
@mamazu
Copy link
Collaborator

mamazu commented Feb 8, 2024

Thanks for your contribution. 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants