Skip to content

Update EntityManager.php#776

Closed
thbourlove wants to merge 1 commit intodoctrine:masterfrom
thbourlove:patch-1
Closed

Update EntityManager.php#776
thbourlove wants to merge 1 commit intodoctrine:masterfrom
thbourlove:patch-1

Conversation

@thbourlove
Copy link

No description provided.

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-2651

We use Jira to track the state of pull requests and the versions they got
included in.

@docteurklein
Copy link
Contributor

I think it has already been discussed many times. EntityManager is not meant to be extended, that's why the final keyword is used :)

You'd better use composition/decorators if you want to extend its functionalities.

@docteurklein
Copy link
Contributor

@thbourlove
Copy link
Author

@docteurklein
Thank you, but I don't understand why comment around the final keyword? It make ctags fail to generate tag for this class. Some other tools(php_CodeSniffer and some vim bundles) also cannot be work well because of this.

@docteurklein
Copy link
Contributor

also, you can't mock it directly. (but you still can mock its interface). It's still not a reason to remove the final keyword.

But, concerning ctags, vim & co, which I use everyday, I don't have problems with it.
Note: I use exuberant-ctags.

@thbourlove
Copy link
Author

@doctrinebot
But I can extend it directly, just because there is comment around the final keyword! So, I think there is no difference between /* final */class EntityManager implements EntityManagerInterface and class EntityManager implements EntityManagerInterface.
Maybe you want final class EntityManager implements EntityManagerInterface ?

@docteurklein
Copy link
Contributor

oh, sorry, I haven't seen it is commented out!

@docteurklein
Copy link
Contributor

See the commit here: acc8b61

@stof
Copy link
Member

stof commented Sep 5, 2013

@thbourlove The class was previously marked as final. It has been commented to allow for mocking, but it has been kept in a comment to show that you should consider it as final. Extending the entity manager is not a supported extension point

@stof
Copy link
Member

stof commented Sep 5, 2013

And if these tools fail to work because of a comment in the file, it means they have a bug and you should report it to their team

@beberlei
Copy link
Member

beberlei commented Sep 8, 2013

@thbourlove Can you add a NOTICE sentence to the docblock of EntityManager discussing that extension is not wanted and you should use the Decorator instead? Then i can merge this.

@thbourlove
Copy link
Author

@beberlei
Is this?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants