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

Fix Incompatibility for doctrine #988

Merged

Conversation

@Baachi
Copy link
Contributor

Baachi commented Nov 23, 2019

Hey guys,

i adjusted the code from #985 and added a test for this class.

#SymfonyHackday

ctrl-f5 and others added 2 commits Nov 22, 2019
@rogamoore

This comment has been minimized.

Copy link

rogamoore commented Nov 26, 2019

👍 Anything preventing from merging and releasing this fix? :)

@Steveb-p

This comment has been minimized.

Copy link
Contributor

Steveb-p commented Nov 27, 2019

@rogamoore I'm usually not the one merging, since I've opted to take care Kafka part of Enqueue, but it seems @makasim is recently pretty busy.

I'll let it "bake" for some time, in case someone has any additional comments (like I've made the assumption that previous PR doing this is OK while it was pointing to wrong class, so I don't want to hastily merge).

At the same time, you can use Composer's functionality to overwrite packages to temporarily provide your own fork instead of a package: https://getcomposer.org/doc/05-repositories.md#loading-a-package-from-a-vcs-repository

Loading a package from a VCS repository
There are a few use cases for this. The most common one is maintaining your own fork of a third party library. If you are using a certain library for your project and you decide to change something in the library, you will want your project to use the patched version. If the library is on GitHub (this is the case most of the time), you can simply fork it there and push your changes to your fork. After that you update the project's composer.json. All you have to do is add your fork as a repository and update the version constraint to point to your custom branch. In composer.json, you should prefix your custom branch name with "dev-".

@makasim

This comment has been minimized.

Copy link
Member

makasim commented Nov 27, 2019

It looks good. One thing though: Doctrine version has to be updated in composer.json.

@Steveb-p I don't use the library nor work with PHP these days. Feel free to take over it (if you have that desire of course).

@Steveb-p

This comment has been minimized.

Copy link
Contributor

Steveb-p commented Nov 27, 2019

@makasim I can take it over if you're not working with it anymore. As we discussed in another issue (the one where you were looking for maintainers) I'll keep directly maintaining only the part with Kafka, since that's the package I'm familiar with, but will take a little more active approach with PR's from other packages.

I'll check/review this PR in more detail tonight after work @rogamoore

@makasim

This comment has been minimized.

Copy link
Member

makasim commented Nov 27, 2019

Thank you @Steveb-p

@rogamoore

This comment has been minimized.

Copy link

rogamoore commented Nov 28, 2019

Regarding the Doctrine version I think this fix is still compatible with Doctrine Bundle <2.0 because it's just an alias that was removed, so using the Doctrine\Common\Persistence\ManagerRegistry directly should have been possible already before.

@makasim makasim merged commit ddec78e into php-enqueue:master Nov 28, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alcaeus

This comment has been minimized.

Copy link

alcaeus commented Nov 28, 2019

Regarding the Doctrine version I think this fix is still compatible with Doctrine Bundle <2.0 because it's just an alias that was removed, so using the Doctrine\Common\Persistence\ManagerRegistry directly should have been possible already before.

That is correct - this uses the base interface for all manager registries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.