-
-
Notifications
You must be signed in to change notification settings - Fork 504
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
Deprecation and forward compat for flush #2159
Conversation
} | ||
if (func_num_args() === 2) { | ||
@trigger_error( | ||
'$options will become a first argument in doctrine/mongodb-odm 2.0, you may pass it as first now.', |
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.
I'm not sure about wording here
E_USER_DEPRECATED | ||
); | ||
} | ||
if (func_num_args() === 2) { |
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.
We should only trigger the deprecation notice if documents were passed:
if (func_num_args() === 2) { | |
elseif (func_num_args() === 2) { |
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 necessarily, $dm->flush(null, $options);
will stop working as well and folks can call $dm->flush($options);
now
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.
As is, $dm->flush([$document], $options);
throws two deprecation warnings. We should make sure this doesn't happen as it might confuse the user.
As I write this, we should also make sure to gracefully handle $dm->flush([], $options);
to make sure we don't treat it as the "user passed documents" case.
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.
It seems that passing an array of documents is an undocumented feature. I didn't find anything in the documentation and the method signature mentions just one object. Only deep down in the UoW is the check if the parameter is an array. So did anyone even use that?
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.
Hard to say, but there's a chance :)
} | ||
if (func_num_args() === 2) { | ||
@trigger_error( | ||
'$options will become a first argument in doctrine/mongodb-odm 2.0, you may pass it as first now.', |
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.
'$options will become a first argument in doctrine/mongodb-odm 2.0, you may pass it as first now.', | |
sprintf('Flushing selected documents has been deprecated and will be removed in doctrine/mongodb-odm 2.0. You can omit the first argument to %s.', __METHOD__), |
e253a63
to
83b6968
Compare
83b6968
to
fd955b6
Compare
We missed this originally. Patch also includes forward compatibility layer so consumers may pass
$options
as a first argument already. Also there's a chance I should have placed similar code inUnitOfWork::commit
although I haven't thought about any legit reason to call it directly instead ofDocumentManager::flush
so I didn't bother