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

Deprecate legacy commit options #2578

Merged
merged 2 commits into from
Nov 8, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 4 additions & 11 deletions docs/en/reference/working-with-objects.rst
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,8 @@ Flush Options
-------------

When committing your documents you can specify an array of options to the
``flush`` method. With it you can send options to the underlying database
like ``safe``, ``fsync``, etc.
``flush`` method. You can use this to pass a write options to the underlying
operations, e.g. a custom write concern.

Example:

Expand All @@ -145,7 +145,7 @@ Example:
$user = $dm->getRepository(User::class)->find($userId);
// ...
$user->setPassword('changeme');
$dm->flush(null, ['safe' => true, 'fsync' => true]);
$dm->flush(['writeConcern' => new \MongoDB\Driver\WriteConcern(1)]);

You can configure the default flush options on your ``Configuration`` object
if you want to set them globally for all flushes.
Expand All @@ -157,16 +157,9 @@ Example:
<?php

$config->setDefaultCommitOptions(
[
'safe' => true,
'fsync' => true
]
['writeConcern' => new \MongoDB\Driver\WriteConcern(1)]
);

.. note::

Safe is set to true by default for all writes when using the ODM.

Removing documents
------------------

Expand Down
14 changes: 14 additions & 0 deletions lib/Doctrine/ODM/MongoDB/Configuration.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
use Psr\Cache\CacheItemPoolInterface;
use ReflectionClass;

use function array_key_exists;
use function interface_exists;
use function trigger_deprecation;
use function trim;
Expand Down Expand Up @@ -449,6 +450,19 @@ public function getDefaultCommitOptions(): array
/** @psalm-param CommitOptions $defaultCommitOptions */
public function setDefaultCommitOptions(array $defaultCommitOptions): void
{
foreach (UnitOfWork::DEPRECATED_WRITE_OPTIONS as $deprecatedOption) {
if (! array_key_exists($deprecatedOption, $defaultCommitOptions)) {
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to flip this logic and avoid the continue? This isn't a matter of nesting conditionals, so I think we could go either way here.

Up to you.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is possible with a small change to the phpcs configuration. The early exit is now no longer required if there's only a single if in the scope. Good judgment should still be applied :)

continue;
}

trigger_deprecation(
Copy link
Member

Choose a reason for hiding this comment

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

TIL symfony/deprecation-contracts exists. Is this something we might want to use in PHPLIB down the line? Or do you think we should stick with the forthcoming logging API?

Copy link
Member

Choose a reason for hiding this comment

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

FWIW there's also https://github.com/doctrine/deprecations but we never changed ODM (we were using Symfony's stuff before Doctrine's was introduced)

Copy link
Member Author

Choose a reason for hiding this comment

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

@jmikola I honestly haven't considered that at all yet - but then on the other hand it's just a wrapper around @trigger_error with some formatting sugar.

@malarzm and I'm very much against changing it at this point ;)

Copy link
Member

Choose a reason for hiding this comment

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

I see no benefits either so no worries ;)

'doctrine/mongodb-odm',
'2.6',
'The "%s" commit option used in the configuration is deprecated.',
$deprecatedOption,
);
}

$this->attributes['defaultCommitOptions'] = $defaultCommitOptions;
}

Expand Down
18 changes: 18 additions & 0 deletions lib/Doctrine/ODM/MongoDB/UnitOfWork.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
use UnexpectedValueException;

use function array_filter;
use function array_key_exists;
use function assert;
use function count;
use function get_class;
Expand All @@ -42,6 +43,7 @@
use function serialize;
use function spl_object_hash;
use function sprintf;
use function trigger_deprecation;

/**
* The UnitOfWork is responsible for tracking changes to objects during an
Expand Down Expand Up @@ -88,6 +90,9 @@ final class UnitOfWork implements PropertyChangedListener
*/
public const STATE_REMOVED = 4;

/** @internal */
public const DEPRECATED_WRITE_OPTIONS = ['fsync', 'safe', 'w'];
Copy link
Member

Choose a reason for hiding this comment

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

This makes me wonder how w interacts with an explicit writeConcern option, but that's beyond the scope of this deprecation PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

😅


/**
* The identity map holds references to all managed documents.
*
Expand Down Expand Up @@ -393,6 +398,19 @@ public function setDocumentPersister(string $documentName, Persisters\DocumentPe
*/
public function commit(array $options = []): void
{
foreach (self::DEPRECATED_WRITE_OPTIONS as $deprecatedOption) {
if (! array_key_exists($deprecatedOption, $options)) {
continue;
}

trigger_deprecation(
'doctrine/mongodb-odm',
'2.6',
'The "%s" commit option is deprecated.',
$deprecatedOption,
);
}

// Raise preFlush
$this->evm->dispatchEvent(Events::preFlush, new Event\PreFlushEventArgs($this->dm));

Expand Down