-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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 deprecation warnings for 2.x #6869
Conversation
composer.json
Outdated
@@ -31,7 +31,8 @@ | |||
"doctrine/coding-standard": "^1.0", | |||
"phpunit/phpunit": "^6.0", | |||
"squizlabs/php_codesniffer": "^3.1", | |||
"symfony/yaml": "~3.4|~4.0" | |||
"symfony/phpunit-bridge": "^3.3|^4.0", | |||
"symfony/yaml": "4.0.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just temporary hack to work around symfony/symfony#25405 on CI.
9ab6c3c
to
68e7d06
Compare
lib/Doctrine/ORM/EntityManager.php
Outdated
@@ -353,6 +353,13 @@ public function createQueryBuilder() | |||
*/ | |||
public function flush($entity = null) | |||
{ | |||
if (func_num_args() === 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using > 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't $entity !== null
simpler? I mean, only the first argument is used anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah even better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would still allow flush(null)
though. I'll change it to !== 0
.
lib/Doctrine/ORM/EntityManager.php
Outdated
*/ | ||
public function detach($entity) | ||
{ | ||
@trigger_error('Method ' . __METHOD__ . '() is deprecated and will be removed in Doctrine 3.0, please migrate to ' . self::class . '::clear().', E_USER_DEPRECATED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linebreak here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, EntityManager#clear()
has a different behaviour... and I'm not even sure if we should point people to do partial cleaning of the UoW, it might have the very same issues we have with EntityManager#detach()
.
@@ -44,6 +44,8 @@ class YamlDriver extends FileDriver | |||
*/ | |||
public function __construct($locator, $fileExtension = self::DEFAULT_FILE_EXTENSION) | |||
{ | |||
@trigger_error('YAML mapping driver is deprecated and will be removed in Doctrine 3.0, please migrate to annotation or XML driver.', E_USER_DEPRECATED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linebreak?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just some minor comments.
We just need to keep in mind that our plan is to release 2.6 WITHOUT the deprecation notices. 2.7 would be the one adding them.
lib/Doctrine/ORM/EntityManager.php
Outdated
@@ -353,6 +353,13 @@ public function createQueryBuilder() | |||
*/ | |||
public function flush($entity = null) | |||
{ | |||
if (func_num_args() === 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't $entity !== null
simpler? I mean, only the first argument is used anyway
@@ -26,6 +26,8 @@ | |||
* | |||
* @author Roman Borschel <[email protected]> | |||
* @since 2.0 | |||
* | |||
* @deprecated This class is deprecated and will be removed in Doctrine 3.0, proxies will no longer implement it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to mention which interface should be used? So that people can already start preparing their stuff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this one, we would be referencing 3rd party code (ProxyManager) which is not required in Composer.
lib/Doctrine/ORM/EntityManager.php
Outdated
*/ | ||
public function detach($entity) | ||
{ | ||
@trigger_error('Method ' . __METHOD__ . '() is deprecated and will be removed in Doctrine 3.0, please migrate to ' . self::class . '::clear().', E_USER_DEPRECATED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, EntityManager#clear()
has a different behaviour... and I'm not even sure if we should point people to do partial cleaning of the UoW, it might have the very same issues we have with EntityManager#detach()
.
Well, the sooner we ship them, the better for users, but if there are other reasons, 2.7 is fine by me. |
68e7d06
to
ff822e0
Compare
Updated, added deprecations for codegen (#6870). |
93e5747
to
278edf9
Compare
I've also added deprecatons for:
Also partially imported UPGRADE notes from develop and reworded them (no mapping-related changes since it's unfinished). |
UPGRADE.md
Outdated
@@ -1,3 +1,103 @@ | |||
# Upgrade to 2.7 | |||
|
|||
## BC Break: Deprecated code generators and related console commands |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprecations are not BC-breaks, are they?
@@ -70,6 +70,8 @@ public function setProxyDir($dir) | |||
* Gets the directory where Doctrine generates any necessary proxy class files. | |||
* | |||
* @return string|null | |||
* | |||
* @deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not also trigger a deprecation error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no replacement, it would be an error that cannot be muted/fixed, it would be just annoying without benefit.
f74c575
to
0be38dd
Compare
0be38dd
to
5b775da
Compare
4caef22
to
4257a99
Compare
…ethod Deprecation of EntityManager copy method
4257a99
to
d3f80eb
Compare
Rebased. |
I think deprecations for named queries should be added as well. |
Based on UPGRADE document for 3.0.
Deprecations added for:
I have not added any notes about mapping/metadata changes here because it's still pretty unstable.
Tests marked as
@legacy
use any of the deprecated methods.Added explicit deprecation tests for deprecated methods.
I'd like to get these into 2.6, there is no reason to delay it.