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

EntityManager decorator #22

Merged
merged 8 commits into from Nov 2, 2018

Conversation

3 participants
@MartkCz
Copy link
Contributor

MartkCz commented Jun 14, 2018

If someone use Doctrine\ORM\EntityManager instead of Nettrine\ORM\EntityManager type hint it's BC break. I can add workaround to OrmExtension

MartkCz added some commits Jun 14, 2018

@f3l1x

This comment has been minimized.

Copy link
Member

f3l1x commented Jun 14, 2018

Thank you. I would prefer to release v0.2 and after all, drop EntityManager and introduce only decorator. Do you agree?

@MartkCz

This comment has been minimized.

Copy link
Contributor Author

MartkCz commented Jun 15, 2018

Yes, I have not problem with it

class EntityManager extends DoctrineEntityManager
class EntityManager extends EntityManagerDecorator implements IEntityManager

This comment has been minimized.

@f3l1x

f3l1x Jun 15, 2018

Member

Could we rename it to EntityManagerDecorator and drop implements?

// Entity Manager
$builder->addDefinition($this->prefix('entityManager'))
->setFactory($entityManagerClass, [$original]);

This comment has been minimized.

@f3l1x

f3l1x Jun 15, 2018

Member

Is it still needed? Could we drop it?

This comment has been minimized.

@MartkCz

MartkCz Jun 15, 2018

Author Contributor

It's EntityManagerDecorator registration

@@ -19,7 +19,7 @@
/** @var mixed[] */
private $defaults = [
'entityManagerClass' => EntityManager::class,
'entityManagerClass' => EntityManagerDecorator::class,

This comment has been minimized.

@f3l1x

f3l1x Jun 15, 2018

Member

What about:

  • entityManagerDecoratorClass
  • decoratorClass
  • decorator
$builder->addDefinition($this->prefix('entityManager'))
->setType($entityManagerClass)
->setFactory(EntityManagerFactory::class . '::create', [
$original = $builder->addDefinition($this->prefix('entityManager'))

This comment has been minimized.

@f3l1x

f3l1x Jun 15, 2018

Member

Could we drop entityManager, right?

This comment has been minimized.

@f3l1x

f3l1x Jun 15, 2018

Member

Oh I see.

This comment has been minimized.

@f3l1x

f3l1x Jun 15, 2018

Member

It's needed for decorator.

class DummyEntityManager extends EntityManager
class DummyEntityManager extends EntityManagerDecorator

This comment has been minimized.

@f3l1x

f3l1x Jun 18, 2018

Member

DummyEntityManagerDecorator

@f3l1x

This comment has been minimized.

Copy link
Member

f3l1x commented Jun 18, 2018

I think it's done. What do you think @MartkCz?

@MartkCz

This comment has been minimized.

Copy link
Contributor Author

MartkCz commented Jun 18, 2018

I agree

@f3l1x

This comment has been minimized.

Copy link
Member

f3l1x commented Jun 28, 2018

One last thing, we should add some note/example into readme. How people should you EntityManager (over EntityManagerDecorator).

Could you pleasae @MartkCz?

@MartkCz

This comment has been minimized.

Copy link
Contributor Author

MartkCz commented Jun 28, 2018

Maybe rename section EntityManager -> EntityManagerDecorator? I'm not sure,

@f3l1x

This comment has been minimized.

Copy link
Member

f3l1x commented Jun 28, 2018

Definitely.

@f3l1x

This comment has been minimized.

Copy link
Member

f3l1x commented Jul 19, 2018

Sorry for delay, I will try to test it as soon as possible.

@f3l1x f3l1x added this to the v0.3 milestone Nov 2, 2018

@f3l1x f3l1x merged commit d4c006e into nettrine:master Nov 2, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.5%) to 45.638%
Details
@f3l1x

This comment has been minimized.

Copy link
Member

f3l1x commented Nov 2, 2018

Done. :-) Thanks.

@JanMikes

This comment has been minimized.

Copy link
Contributor

JanMikes commented Nov 21, 2018

Hi, i am very unhappy about this PR as it caused BC break on many of our projects - we are using Doctrine\ORM\EntityManager for our dependencies instead of Nettrine\ORM\EntityManager which i consider to be better.

@f3l1x

This comment has been minimized.

Copy link
Member

f3l1x commented Nov 21, 2018

We can change signiture for this class. Or you can do also.

@JanMikes

This comment has been minimized.

Copy link
Contributor

JanMikes commented Nov 21, 2018

Sorry i am not sure i understand what do you mean.

So far we have fixed it by replacing Doctrine\ORM\EntityManager with Doctrine\ORM\EntityManagerInterface which is most probably the best practice.

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