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

Added: ability to choose different entity manager #1081

Merged
merged 13 commits into from
Nov 24, 2020
Merged

Added: ability to choose different entity manager #1081

merged 13 commits into from
Nov 24, 2020

Conversation

balabis
Copy link
Contributor

@balabis balabis commented Aug 20, 2020

Fixed issues of #1042

@Steveb-p
Copy link
Contributor

LGTM. All that is left is for @makasim to review.

@@ -171,6 +171,14 @@ private function registerJobQueueDoctrineEntityMapping(ContainerBuilder $contain
return;
}

foreach ($container->getExtensionConfig('enqueue') as $modules) {
foreach ($modules as $config) {
if (isset($config['job']) && false === $config['job']['default_mapping']) {
Copy link
Member

Choose a reason for hiding this comment

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

That does not look accurate. For example one config defines false and the next defines true. The merged value might be true but this logic would not load default entites.

There must be a method that allows getting merged version of the config. But I am not sure. Could you please look for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for taking so long.
I was not able to find a solution for this yet, but this part of code is happening before the merging of configuration, no?

Copy link
Member

Choose a reason for hiding this comment

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

You should merge it manually and use the result in condition.

Copy link
Member

Choose a reason for hiding this comment

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

Symfony provides tools for that.

Copy link
Member

Choose a reason for hiding this comment

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

I believe

Copy link
Contributor

@Steveb-p Steveb-p Sep 7, 2020

Choose a reason for hiding this comment

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

https://symfony.com/doc/current/bundles/configuration.html#processing-the-configs-array

EDIT: However I'm surprised that the configuration isn't already resolved at this point. Processing it more than once should not be an issue, since in production it would be cached if I understand correctly, but still...

@balabis balabis requested a review from makasim September 8, 2020 07:04
@balabis
Copy link
Contributor Author

balabis commented Sep 11, 2020

Hey @Steveb-p , maybe you know what exactly causes the tests to fail?

TypeError: Argument 1 passed to Enqueue\Doctrine\DoctrineConnectionFactoryFactory::__construct() must be an instance of Doctrine\Common\Persistence\ManagerRegistry, instance of Double\stdClass\P1 given

Before the last code fix, all tests passed, but the DoctrineConnectionFactoryFactory constructor argument was the same as it is now, though locally, I can see that the 1st argument passed to the constructor is Double\ManagerRegistry\P1

@stale
Copy link

stale bot commented Oct 11, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Oct 11, 2020
@stale stale bot removed the wontfix label Oct 14, 2020
@stale
Copy link

stale bot commented Nov 14, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Nov 14, 2020
@stale stale bot removed the wontfix label Nov 14, 2020
@balabis
Copy link
Contributor Author

balabis commented Nov 24, 2020

hey @makasim, would you mind reviewing this PR one more time?

@makasim makasim merged commit 9010c31 into php-enqueue:master Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants