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

Expose the library version to support deprecations #558

Closed
fgm opened this issue Jul 5, 2018 · 4 comments
Closed

Expose the library version to support deprecations #558

fgm opened this issue Jul 5, 2018 · 4 comments

Comments

@fgm
Copy link

fgm commented Jul 5, 2018

Version 1.4.0 introduced a pair of deprecations (Collection::count() and ChangeStream::CURSOR_NOT_FOUND), which cause problems in code using the library because compatibility with earlier versions may require non-identical code. For example Collection::countDocuments() and Collection::estimatedDocumentCount() are two replacement for the deprecated Collection::count() depending on the use case, but code that needs to be compatible with both 1.2/1.3 and 1.4 cannot use either with earlier versions of the library since both were introduced in 1.4.0, so code needs to be aware of the actual library version to decide on the API to use.

This is usable by hackish means like using reflection on cout() to check the doccomment for
@deprecated, or other means, but it would be simpler if the code could just use a version constant or API version constant, so I suggest the library starts exposing such a constant.

@jmikola
Copy link
Member

jmikola commented Jul 5, 2018

The @deprecated annotations added in 1.4.0 are purely for documentation purposes. I'll address each point individually:

  • ChangeStream::CURSOR_NOT_FOUND was inadvertently made public in the 1.3.0 but was never advertised for public use (or documented). That said, deleting it would certainly break BC for anyone that did happen to reference it (for whatever reason). Adding a comment to note our intention to remove it in the next major release seemed best.
  • Collection::count() was also only deprecated in documentation (both doc block and in the manual). This case is unique, as it relates to the command being deprecated in the MongoDB server and across all other drivers due to its inherent incompatibility with transactions. We have no plans to actually remove this function from the PHP library.

You can contrast these annotations with actual deprecations in Collection::find() and Database::createCollection(), where we raise E_USER_DEPRECATED warnings if certain options are used. In these cases, it is more vital that users upgrade their application to stop using those APIs so we elected to be noisier than a simple documentation entry.

code that needs to be compatible with both 1.2/1.3 and 1.4 cannot use either with earlier versions of the library since both were introduced in 1.4.0

Between count(), countDocuments(), and estimatedDocumentCount(), only countDocuments() can be used within a transaction. The count() and estimatedDocumentCount() methods utilize the count command and cannot be executed within a transaction (the server will raise an error). Therefore, I don't see how this relates to "code that needs to be compatible with both 1.2/1.3 and 1.4". Any code written to make use of transactions will be inherently incompatible with code written for an earlier library version (library versions before 1.3 also did not support a session option on operations, which is also required to associate ops with a transaction).

If you're use case has nothing to do with transactions, I would suggest sticking with count() in code that MUST be compatible with multiple versions of the library. estimatedDocumentCount() is simply count() without support for a query filter, limit, or skip options. If at some point you decide to raise the library requirement to 1.4+, you can then replace all usage of count() with estimatedDocumentCount() or countDocuments() accordingly (depending on whether you need to use said options).

Checking for the availability of new APIs is easily accomplished with method_exists(). This is a common approach and arguably more direct than checking version numbers. The Symfony polyfill library uses this to great effect to backport newer APIs for older PHP versions.


We previously considered implementing a version constant in PHPLIB-131 but ultimately decided against it. Rather than rehash the reasons here, I'd encourage you to check the ticket and the related discussion in #280.

@fgm
Copy link
Author

fgm commented Jul 5, 2018

Thanks for the fast and detailed explanation.

The needs for 1.2/1.3/1.4 compatibility comes from the fact that the code (the drupal module) needs to work on environments (like macOS X) where version 1.4 cannot be required because it doesn't support the PHP extension versions available in the builtin or Homebrew PHP. So at any given time, the module may have to run on any version at or above 1.2.0 : this has indeed nothing to do with transactions.

The problem with maintaining use of count() is that it increases the reported error count in static analysis reports because of the @deprecated tag.

Just read PHPLIB-131 and #280. Thanks for the information. Guess I'll have to use one of the suggested workarounds. FWIW, my current logic is at https://github.com/fgm/mongodb/pull/27/files?utf8=%E2%9C%93&diff=split&w=1#diff-213bffb5b19104d56561eea15854f864R47

@jmikola
Copy link
Member

jmikola commented Jul 5, 2018

needs to work on environments (like macOS X) where version 1.4 cannot be required because it doesn't support the PHP extension versions available in the builtin or Homebrew PHP.

Extension version 1.5.0 is absolutely supported on macOS. The more general issue is that Homebrew itself removed all formula for PECL extensions when the PHP tap was deprecated a few months ago. We've since updated the install instructions to advise macOS users to instead install the driver with pecl install directly.

The problem with maintaining use of count() is that it increases the reported error count in static analysis reports because of the @deprecated tag.

In the interest of satisfying static analysis, does the tool support any way to disable checks for a segment of code? For example, Scrutinizer CI allows annotations to suppress false positives.

FWIW, my current logic is at https://github.com/fgm/mongodb/pull/27/files?utf8=%E2%9C%93&diff=split&w=1#diff-213bffb5b19104d56561eea15854f864R47

I think that method can be improved with something like:

if ( ! empty(static::$libraryVersion)) {
    return static::$libraryVersion;
}

if (method_exists(Collection::class, 'countDocuments')) {
    return (static::$libraryVersion = '1.4.0');
}

if (method_exists(Collection::class, 'watch')) {
    return (static::$libraryVersion = '1.3.0');
}

return (static::$libraryVersion = '1.2.0');

This would remove the overhead of reflection and any doc block parsing. Even without this approach, there should be no reason to parse count()'s doc block for a deprecation notice when you can just check for the existence of a countDocuments() method.

@fgm
Copy link
Author

fgm commented Jul 5, 2018

Thanks for this very nice simplification, and the Pecl note.

@fgm fgm closed this as completed Jul 5, 2018
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

No branches or pull requests

2 participants