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

Remove doctrine managers #494

Closed
wants to merge 1 commit into from

Conversation

EmmanuelVella
Copy link
Contributor

It seems there is no reason to keep these managers ?

@rande
Copy link
Member

rande commented Feb 17, 2014

You are probably right, this is supposed to save the media into the database. But now this is handled by doctrine's event.

@core23
Copy link
Member

core23 commented Feb 13, 2016

Please rebase @EmmanuelVella

@EmmanuelVella EmmanuelVella force-pushed the managers branch 3 times, most recently from b446c1d to 779946d Compare February 22, 2016 14:26
@EmmanuelVella
Copy link
Contributor Author

@core23 Rebase done (but I can't test as I'm not using this bundles anymore).

@core23
Copy link
Member

core23 commented Feb 22, 2016

Thanks @EmmanuelVella

Maybe we can leave both files and add a deprecation notice instead?

@EmmanuelVella
Copy link
Contributor Author

@core23 That a good idea. I have updated my PR.

@@ -19,6 +19,8 @@
/**
* this method overwrite the default AdminModelManager to call
* the custom methods from the dedicated media manager.
*
* @deprecated
Copy link
Member

Choose a reason for hiding this comment

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

Can you please leave a comment here?

@deprecated since XXX (to be removed in 3.0)

@EmmanuelVella
Copy link
Contributor Author

Done !

@core23
Copy link
Member

core23 commented Feb 25, 2016

Users need to know since when they are deprecated and when they will be removed. @EmmanuelVella

@core23
Copy link
Member

core23 commented Mar 18, 2016

ping @EmmanuelVella

@EmmanuelVella
Copy link
Contributor Author

I have updated the deprecated message.

@@ -19,6 +19,8 @@
/**
* this method overwrite the default AdminModelManager to call
* the custom methods from the dedicated media manager.
*
* @deprecated This class is deprecated since version 2.4 and will be removed in 3.0.
Copy link
Contributor

Choose a reason for hiding this comment

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

2.4 does not exist.

@soullivaneuh
Copy link
Member

@EmmanuelVella According to sonata-project/SonataAdminBundle#3731, sonata media bundle won't be 2.4 but 3.0.

After that, if those manager are really not used, I think we could accept this BC break for 3.0.

I'll ping you when the 3.x will be ready so you will be able to rework this PR. 👍

@EmmanuelVella
Copy link
Contributor Author

Ho ok I didn't know there won't be a 2.4 version. Waiting for your ping then !

@soullivaneuh
Copy link
Member

Thank you @EmmanuelVella, I cherry-picked your commit with deletion instead of deprecation.

49e628e

@EmmanuelVella
Copy link
Contributor Author

ok 👍

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.

6 participants