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.

Sign up for GitHub

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

Jump to bottom

Deprecate legacy commit options #2578

Merged
merged 2 commits into from
Nov 8, 2023
Merged

Deprecate legacy commit options #2578

merged 2 commits into from
Nov 8, 2023

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented Nov 6, 2023

Q A
Type improvement
BC Break no
Fixed issues

Summary

The fsync, safe, and w options can be deprecated, as they are all handled by the writeConcern commit option.

@alcaeus alcaeus self-assigned this Nov 6, 2023
@alcaeus alcaeus requested a review from jmikola November 6, 2023 13:07
@alcaeus
Copy link
Member Author

alcaeus commented Nov 6, 2023

Note: static analysis errors are handled in #2554

jmikola
jmikola approved these changes Nov 6, 2023
View reviewed changes
Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

Optional suggestion to replace continue with the trigger_deprecation call but I defer to you. LGTM.

lib/Doctrine/ODM/MongoDB/Configuration.php Outdated
@@ -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 :)

lib/Doctrine/ODM/MongoDB/Configuration.php Outdated
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 ;)

lib/Doctrine/ODM/MongoDB/UnitOfWork.php
@@ -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.

😅

@malarzm
Copy link
Member

malarzm commented Nov 6, 2023 •
edited
Loading

writeConcern is not allowed by Symfony's bundle (https://github.com/doctrine/DoctrineMongoDBBundle/blob/4.7.x/DependencyInjection/Configuration.php#L100-L111) so we'll need to make sure it is in next version :) And deprecate w there as well. No clue how Laminas' module is handling this.

@alcaeus
Copy link
Member Author

alcaeus commented Nov 8, 2023

writeConcern is not allowed by Symfony's bundle (https://github.com/doctrine/DoctrineMongoDBBundle/blob/4.7.x/DependencyInjection/Configuration.php#L100-L111) so we'll need to make sure it is in next version :) And deprecate w there as well. No clue how Laminas' module is handling this.

I'll take care of the bundle separately - I need to think about how I can allow users to configure a WriteConcern object without having to create a service for it.

@alcaeus alcaeus merged commit aadcca0 into doctrine:2.6.x Nov 8, 2023
@alcaeus alcaeus deleted the deprecate-legacy-write-options branch November 8, 2023 12:42
@malarzm
Copy link
Member

malarzm commented Nov 8, 2023

I need to think about how I can allow users to configure a WriteConcern object without having to create a service for it.

I think you could try having something along:

parameters:
    # ...
    writeConcern:
        w: int
        # ...

and then in Configuration class remap $parameters['writeConcern'] = new WriteConcern(...$parameters['writeConcern']). This would allow to easily add new options, should they appear in WriteConcern itself :)

alcaeus added a commit that referenced this pull request Nov 24, 2023
@alcaeus
Merge branch '2.6.x' into 2.5.x-merge-up-into-2.6.x_tl7iY60u
f7af05a 
* 2.6.x: (30 commits)
  Remove API link from README
  Aggregation uses CursorInterface instead of Cursor
  Deprecate legacy commit options (#2578)
  Improve handling of circular type reference errors in Phpstan
  Add missing phpdoc
  Fix return type for overridden methods
  Fix phpstan errors
  Add sort operator to $search stage
  Bump actions/upload-artifact from 2 to 3 (#2572)
  Implement ObjectManager::isUninitializedObject (#2569)
  Use PHPUnit 10 (#2564)
  Test Symfony 7
  Support Symfony 7 by adding return types conditionally
  PHPCS is now happy with new syntax
  Stop suppressing UndefinedAttributeClass
  Drop support for old symfony components
  ReferenceMany: insert an empty array
  feat: Symfony 7 support
  Remove unusable Match classes
  Require PHP 8.1
  ...
alcaeus added a commit that referenced this pull request Nov 24, 2023
@alcaeus
Merge branch '2.6.x' into 2.5.x-merge-up-into-2.6.x_tl7iY60u
bb45088 
* 2.6.x: (32 commits)
  Deprecate DocumentRepository::clear (#2584)
  Support minDistance and maxDistance options for $near and $nearSphere operators (#2583)
  Remove API link from README
  Aggregation uses CursorInterface instead of Cursor
  Deprecate legacy commit options (#2578)
  Improve handling of circular type reference errors in Phpstan
  Add missing phpdoc
  Fix return type for overridden methods
  Fix phpstan errors
  Add sort operator to $search stage
  Bump actions/upload-artifact from 2 to 3 (#2572)
  Implement ObjectManager::isUninitializedObject (#2569)
  Use PHPUnit 10 (#2564)
  Test Symfony 7
  Support Symfony 7 by adding return types conditionally
  PHPCS is now happy with new syntax
  Stop suppressing UndefinedAttributeClass
  Drop support for old symfony components
  ReferenceMany: insert an empty array
  feat: Symfony 7 support
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@malarzm malarzm malarzm left review comments

@jmikola jmikola jmikola approved these changes

Assignees

@alcaeus alcaeus

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alcaeus @malarzm @jmikola